Add option to adjust Scalebar width #504
Conversation
Added additional input for height. Added necessary properties to the views. Reordered the panel_form to make space for input. Known Issue: Site load show Empty string passed to getElementbyId(). This will also be returned multiple times when the scalebar is shown / hidden. Unclear where this is coming from
Add correct margin specification for top positioned scalebar.
|
This pull request has been mentioned on Image.sc Forum. There might be relevant details there: |
Achieved by querying the new scalebar property and passing the width to draw_scalebar_line(). For correct positioning, offset parameters for the scalebar and the label are introduced. Seems to work for PDF in all scalebar positions and a variety of width/length combinations. Known Issue: TIFF export not working yet.
|
PDF export seems to be working as well with modifactions to I haven't fully understand why the TIFF export breaks with the same parameters ... more investigation to do |
…th internal type casting in JS
adjusting the draw_scalebar_line routine to loop from (-height/2:height/2) which together with the offset introduced in @adc018c0e8b61b761cde8f039c7d6be4bdd5f965 makes sure that the scalebar will always fit
|
And with 1cd3f56 exporting tiffs now also works on a first glance:
|
|
After some further research: E.g: The second link and some other StackOverflow threads often involve data-targets pointing to |
There was a problem hiding this comment.
@JulianHn, thanks for the PR - you've done well to figure all that out!
Just a couple of small points...
The buttons and form elements are slightly bigger than before. You need to add the btn-sm and input-sm classes to them.
Before/after comparison:
Minor point: I wonder if you could get the form to fit on 2 rows instead of 3? If it's too painful don't worry. But if you could get the show/hide button to float right (so as to align with the other forms) that would be great (pull-right class).
It seems that adding an extra text input to the form has disabled the "submit on Enter" behaviour for the length input (and the height input). So that when you enter a length or hight and hit Enter, nothing happens. Previously I was getting the "submit" event for free! Just looked this up at https://www.tjvantoll.com/2013/01/01/enter-should-submit-forms-stop-messing-with-that/
So we need to add that behaviour explicitly. This will do it...
--- a/src/js/views/scalebar_form_view.js
+++ b/src/js/views/scalebar_form_view.js
@@ -27,6 +27,14 @@ var ScalebarFormView = Backbone.View.extend({
"click .pixel_size_display": "edit_pixel_size",
"keypress .pixel_size_input" : "enter_pixel_size",
"blur .pixel_size_input" : "save_pixel_size",
+ "keyup input[type='text']" : "handle_keyup",
+ },
+
+ handle_keyup: function (event) {
+ // If Enter key - submit form...
+ if (event.which == 13) {
+ this.update_scalebar();
+ }
},
The PDF and TIFF export look great. However I noticed an existing issue that could do with improving (while we're here): In the PDF (right) the label is probably a bit too close to the scalebar: it looks OK on the TIFF. Could you give it a bit more space?
Cheers!
|
Oh, and maybe remove the |
Add Unit to the font size. Add correct button/input classes to make sizing consistent. Float submit button to the right
to reenable Enter based submission of the form. Remove form from css selector used in update_scalebar to reflect changes of template. Without this change updating the scalebar does not work when the submit button is in "Show" state.
5 Seems to work as an offset in general for both.
|
Hey @will-moore
All other requested changes are implemented and Enter submission is now working again by applying your patch and fixing a css selector. The label offset in PDF is now consistent with the one in TIFF, I just used the 5 offset that was already implemented there for both now. PDF: TIFF: I have undrafted the PR, as I think it should now have all features and I have done a bit more UI testing without encountering more issues. // Julian |
Merging some changes to improve the scalebar-form layout some more
|
I'm trying to get this included in our daily build, but something's preventing that e.g. snoopycrimecop@ac6d076 |
|
The PR inclusion will be conditional to its status. I suspect the issue came from the default requirement to approve GitHub workflows for first-time contributors. Looking at the actions logs, the initial run four days ago was marked as Regarding the commit, note that https://merge-ci.openmicroscopy.org/jenkins/job/OMERO-plugins-push/ did not run at all on March 27th so this PR did not have a change to be included yesterday. The nightly CI run seems to have successfully included it into the merge build as expected - see snoopycrimecop@0948e32 |
|
@sbesson Ah - thanks, sorry I'd missed that the CI didn't run yesterday. |
Keyup event handling was mistakenly commented out at some point. Add back in. After form template rewrite, CSS selector in scalebar_from_view.update_scalebar() was not working anymore, because it selects the wrapping div instead of the form. This caused spurious buggy behaviour (scalebar only changeable when hidden). Change selector to select by class, which is unique to the scalebar_form.
|
@sbesson the last snoopy merge commit (last night) snoopycrimecop@1cd7091 again failed to merge this PR because I would have expected to only need to approve once? |
|
Hmmm - now "Approve and run" gives me a 500 error at #504 so will try again later... |
|
@sbesson I'm still getting this PR excluded - just now: snoopycrimecop@c6c6834 this time with |
|
Looks like the latest workflow run hit some upstream service unavailability issue |
|
@sbesson I can't actually work out how to get to that view of the jobs. That shows: cc @jburel |
|
From https://github.com/ome/omero-figure/actions/runs/4542452995/jobs/8034996020?pr=504, can you click on the |
|
@JulianHn - Apologies: one more thing... Could you update the JSON model NB: I see that 'Format' page is linked from https://omero-guides.readthedocs.io/en/latest/figure/docs/omero_figure_scripting.html but it would be good to have a link from README here too - since I found it hard to trace just now (but that doesn't have to happen in this PR). Cheers! |
|
Just to be sure: Regarding your NB: I actually do not really see much "how to use" in the current README in general. It's more focused on "how to sysadmin / develop" and the "how to use" is not really linked at all. Should the README be extended in general or do you think a prominent link to the readthedocs would be sufficient. // Julian |
|
Yes, just add a single line to the JSON sample. For user guide etc there is a link from the README to https://github.com/ome/omero-figure/blob/master/SUPPORT.md, which then lists other links (which are both re-directs now), so maybe we just need to move the content of that SUPPORT.md onto the front page README and update the links to make it more prominent. |
The new parameter to adjust scalebar thickness was missing a reference in the JSON format documentation. Add it
|
@JulianHn - apologies this has taken a while to get it deployed on our testing server (I was just testing locally before) but it's there now... I just noticed that when I opened a previously-saved figure, the scalebars don't show up on the figure because the scalebar JSON has no This should simply use Probably the best way to handle this in the app is to increment the VERSION at https://github.com/ome/omero-figure/blob/master/src/js/models/figure_model.js#L4 to Then add a block below this section, similar to the handling of addition of scalebar units to version 5: Cheers |
|
Hey @will-moore, sorry for the delay - I was on vacation / conference the last 1.5 weeks. @pwalczysko: While it might not have been introduced here, I can add the necessary changes in this PR, if you prefer the layout in the // Julian |
Thank you very much for this generous suggestion. Yes, we discussed with @will-moore : |
Bump version to 7 to allow automatic insertion of this property in Figures saved from older versions to allow them to correctly load. To ensure compatibility, adjust the figure export routine to use .get with a default value for the height property, so it does not fail, even if no height is present for any reason
|
@sbesson it seems I need to "Approve and run" this PR after every commit to avoid it being excluded by merge-ci? |
|
This is definitely the expected behavior and I don't think there is a built-in feature to automatically approve every new commit - see https://github.blog/2021-04-22-github-actions-update-helping-maintainers-combat-bad-actors/ for background. Once the PR of a new contributor is accepted, the approval should no longer be mandatory for future contributions so this is a one-off requirement but it will apply to every new commit of the first PR. |
|
The loading of pre-existing Figure files is working now. 👍 |
…to prevent errors in case of missing height
…tency with the labels menu. Add JQuery call to setup Bootstrap tooltips everytime the scalebar form is generated. This also makes the rendering of the tooltips consistent with the other tooltips in the menu. Fixes ome#507.
|
I also missed it :) |
|
Thanks @JulianHn - looks good 👍 |
|
|
@will-moore please go ahead with release. This PR introduces a new version of the format. |
|
Is the Font Size widget completely missing now from the Labels ^^^ ? Looks like it... |
|
Sorry, #504 (comment) is not precise - the Font Size box appears when the Label checkbox is checked. All good here.
|
|
released in omero-figure 6.0.0. Thanks @JulianHn |
Hey all,
This is a first draft to Fix #503
I have added an additional input field to the scalebar_form panel and reordered the inputs to make space.
Adjusting the scalebar in the panel_view is done.
Known Issue: Site load show Empty string passed to getElementbyId(). This will also be returned multiple times when the scalebar is shown / hidden. Unclear where this is coming from
RFC: Is that approach in principle okay? If yes, I will try to track done that getElementbyId() thing and take a look on what needs to be modified in the export routines.
// Julian