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 width height button #193

Merged
merged 9 commits into from Mar 28, 2017

Conversation

Projects
None yet
3 participants
@bramalingam
Member

bramalingam commented Jan 24, 2017

This PR adds an extra button to control the aspect ratio of the panels, when the user edits the width/height of the image.

To Test:

Test if toggling the glyphicon-link button changes status of glyphicon-ok-sign to show/hide.
When the glyphicon-ok-sign is hidden, edit width or height of the image. This will just change the width or height of the image (alone).
When the glyphicon-ok-sign is shown, edit width or height of the image. Please check the width/height ratio (aspect ratio) of the panel before editing. Post editing the aspect ratio should be maintained.
Check if the tooltip on the button makes sense.
And as always code review would be useful as well.
The branch includes the code-layout branch as well, and the PR could technically be considered as an independent review of the code-layout PR(#178) from @will-moore
@will-moore @jburel @aleksandra-tarkowska

Conversation trace from closed PR: #187

@will-moore

This comment has been minimized.

Show comment
Hide comment
@will-moore

will-moore Jan 24, 2017

Member

Looks good and is working well.
Minor bug: when the panel is first rendered, you have the green 'check' showing even though the link button is not 'checked'.
You could show/hide the green check purely using the "active" class on the parent button.

  .active .aspect_ratio_selected { display: inline-block }
  .aspect_ratio_selected {
    display: none
    }

Actually it would also be nicer to add the other styles 'color: green' etc to the css .aspect_ratio_selected class instead of in-line styles.

screen shot 2017-01-24 at 12 12 58

Also, as mentioned before, you need to use m.save(newValues) instead of m.set(newValues) to enable the figure "Save" button.

Member

will-moore commented Jan 24, 2017

Looks good and is working well.
Minor bug: when the panel is first rendered, you have the green 'check' showing even though the link button is not 'checked'.
You could show/hide the green check purely using the "active" class on the parent button.

  .active .aspect_ratio_selected { display: inline-block }
  .aspect_ratio_selected {
    display: none
    }

Actually it would also be nicer to add the other styles 'color: green' etc to the css .aspect_ratio_selected class instead of in-line styles.

screen shot 2017-01-24 at 12 12 58

Also, as mentioned before, you need to use m.save(newValues) instead of m.set(newValues) to enable the figure "Save" button.

@will-moore

This comment has been minimized.

Show comment
Hide comment
@will-moore

will-moore Jan 24, 2017

Member

You need to remove the compiled figure.js and templates.js files. I thought the 'merge' had removed them but it seems not.

Member

will-moore commented Jan 24, 2017

You need to remove the compiled figure.js and templates.js files. I thought the 'merge' had removed them but it seems not.

@bramalingam

This comment has been minimized.

Show comment
Hide comment
@bramalingam

bramalingam Jan 25, 2017

Member

@will-moore I have pushed two commits. Please check and let me know if it looks fine.

Member

bramalingam commented Jan 25, 2017

@will-moore I have pushed two commits. Please check and let me know if it looks fine.

@will-moore

This comment has been minimized.

Show comment
Hide comment
@will-moore

will-moore Jan 26, 2017

Member

@bramalingam If I check the 'link', change the Width then move focus away (E.g. click on X input) then the panel is re-rendered, the 'link' button is maintained as '.active' but we've lost the green check.
See my previous comments on how to tie the 'green check' to the .active class via css instead of JavaScript, so you don't need to update them both at the same time in JS.

Also, as mentioned before (twice), you need to use m.save(newValues) instead of m.set(newValues) to enable the figure "Save" button.

Member

will-moore commented Jan 26, 2017

@bramalingam If I check the 'link', change the Width then move focus away (E.g. click on X input) then the panel is re-rendered, the 'link' button is maintained as '.active' but we've lost the green check.
See my previous comments on how to tie the 'green check' to the .active class via css instead of JavaScript, so you don't need to update them both at the same time in JS.

Also, as mentioned before (twice), you need to use m.save(newValues) instead of m.set(newValues) to enable the figure "Save" button.

@jburel

This comment has been minimized.

Show comment
Hide comment
@jburel

jburel Feb 2, 2017

Member

@bramalingam: ready for review?

Member

jburel commented Feb 2, 2017

@bramalingam: ready for review?

@bramalingam

This comment has been minimized.

Show comment
Hide comment
@bramalingam

bramalingam Feb 2, 2017

Member

Yes it is, guess @will-moore has already reviewed the changes but yeah I can re-list on for review tomorrow.

Member

bramalingam commented Feb 2, 2017

Yes it is, guess @will-moore has already reviewed the changes but yeah I can re-list on for review tomorrow.

@jburel

This comment has been minimized.

Show comment
Hide comment
@jburel

jburel Feb 3, 2017

Member

@will-moore could you check the last commits?

Member

jburel commented Feb 3, 2017

@will-moore could you check the last commits?

@will-moore

This comment has been minimized.

Show comment
Hide comment
@will-moore

will-moore Feb 3, 2017

Member

Working nicely now - Just a tiny typo fix for ;title.
Otherwise good to merge.

Member

will-moore commented Feb 3, 2017

Working nicely now - Just a tiny typo fix for ;title.
Otherwise good to merge.

@jburel

This comment has been minimized.

Show comment
Hide comment
@jburel

jburel Mar 28, 2017

Member

@will-moore happy now with the last commit

Member

jburel commented Mar 28, 2017

@will-moore happy now with the last commit

@will-moore

This comment has been minimized.

Show comment
Hide comment
@will-moore

will-moore Mar 28, 2017

Member

Yes, looks good now thanks.

Member

will-moore commented Mar 28, 2017

Yes, looks good now thanks.

@jburel jburel merged commit c00831d into ome:master Mar 28, 2017

1 check passed

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

@jburel jburel added this to the 3.0.0 milestone Jun 28, 2017

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