New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Xywh panel #169

Merged
merged 9 commits into from Sep 5, 2016

Conversation

Projects
None yet
4 participants
@bramalingam
Member

bramalingam commented Aug 30, 2016

Testing instructions :

  1. Try manually manipulating X,Y Width and Height values in the info panel (both single and multi-select cases).
    Check if the changes get updated on the panels.
  2. Try dragging an image from one page to another and check if the X,Y values are reset to 0,0 at the start of every new page.
  3. Multi-select : Select 2 images from 2 different pages (1 image from page 1 and 1 image from page 2) and change X,Y values. Check if the images get aligned to the appropriate X,Y positions within the page that they reside in. (i.e : image 1 is set to X,Y in page 1 , and image 2 is set to the same X,Y in page 2)

@will-moore

@dominikl

This comment has been minimized.

Show comment
Hide comment
@dominikl

dominikl Aug 31, 2016

Member

Looks good from my point of view... Functionality works like expected.

Member

dominikl commented Aug 31, 2016

Looks good from my point of view... Functionality works like expected.

@will-moore

This comment has been minimized.

Show comment
Hide comment
@will-moore

will-moore Aug 31, 2016

Member

Only the x input is type='number'. I noticed this because Chrome won't set '-' for a number and you see an error in console:

screen shot 2016-08-31 at 17 11 59

I think x, y, width & height need to be "number" inputs.
It might be nice to also use an empty string instead of "-" for x,y,w,h (but not for the other attrs) so you don't force the browser to catch the error.

Member

will-moore commented Aug 31, 2016

Only the x input is type='number'. I noticed this because Chrome won't set '-' for a number and you see an error in console:

screen shot 2016-08-31 at 17 11 59

I think x, y, width & height need to be "number" inputs.
It might be nice to also use an empty string instead of "-" for x,y,w,h (but not for the other attrs) so you don't force the browser to catch the error.

var this_json = m.toJSON();
// Format floating point values
_.each(["x", "y", "width", "height"], function(a){
if (this_json[a] != "-") {

This comment has been minimized.

@will-moore

will-moore Aug 31, 2016

Member

I noticed you can remove this if check now, since m.toJSON() won't give you any - values.

@will-moore

will-moore Aug 31, 2016

Member

I noticed you can remove this if check now, since m.toJSON() won't give you any - values.

@will-moore

This comment has been minimized.

Show comment
Hide comment
@will-moore

will-moore Aug 31, 2016

Member

Need to fix the setting of minus values for x & y when not on page 1 (seems to always jump to the previous page).
Also need to disallow width and height to be less than 1 (negative is bad, probably zero is bad too).
You can set a min='1' when you make these into 'number' inputs, but also have to handle this in JavaScript since users can still type in negative numbers.

Member

will-moore commented Aug 31, 2016

Need to fix the setting of minus values for x & y when not on page 1 (seems to always jump to the previous page).
Also need to disallow width and height to be less than 1 (negative is bad, probably zero is bad too).
You can set a min='1' when you make these into 'number' inputs, but also have to handle this in JavaScript since users can still type in negative numbers.

@will-moore

This comment has been minimized.

Show comment
Hide comment
@will-moore

will-moore Sep 5, 2016

Member

Working nicely now. No console errors, re-render bug fixed.
Good to merge.

Member

will-moore commented Sep 5, 2016

Working nicely now. No console errors, re-render bug fixed.
Good to merge.

@jburel jburel merged commit 6313c25 into ome:master Sep 5, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@bramalingam bramalingam referenced this pull request Mar 3, 2017

Merged

Rois from omero2 #190

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment