Skip to content
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

Vite #477

Merged
merged 157 commits into from
Apr 29, 2024
Merged

Vite #477

merged 157 commits into from
Apr 29, 2024

Conversation

will-moore
Copy link
Member

@will-moore will-moore commented Aug 12, 2022

See testing sheet at https://docs.google.com/spreadsheets/d/17jQ1Hws42ob-2AYWaO7-272tKdXDacfdzZCk-xk62Z4/edit#gid=0

This is a major update of omero-figure to update all dependencies to their latest versions, including:

  • jQuery
  • Bootstrap UI framework - including all new bootstrap-icons
  • Underscore
  • Raphael
  • remove jQueryUI and use native range inputs for sliders
  • replace usage of grunt build workflow with https://vitejs.dev/ (don't need global install of grunt now)
  • All JavaScript is now imported ES modules (no global variables etc).
  • Underscore templates are now .template.html so they are recognised as raw strings by vite
  • Colorpicker is removed (EOL) - use browser colorpicker instead
  • ShapeEditor is now included in this repo.

Vite gives us a JS-based dev server during development (similar to iviewer).

New dev workflow - see README.

Testing:

Since this work impacts all of the JavaScript and html code, ALL that functionality will need testing:

  • Every button, menu-option, dialog, form element etc needs to be checked.
  • I don't know that we have testing scenarios for most of the app. Only for opening, saving figures.

@will-moore
Copy link
Member Author

@Tom-TBT - I have added click-handling so the range sliders (channels and z-projection) should behave the same as the did before with jQuery sliders - The handle nearest to the point you clicked is moved to where you clicked.

@pwalczysko
Copy link
Member

pwalczysko commented Apr 5, 2024

Sorry, deleted the comment which was reporting a bug of copy and paste of ROIs. This was caused by me using a smaller screenshot of the same image - in fact, the behaviour is as expected when pasting onto an image of the same size as was the intention.

@will-moore will-moore mentioned this pull request Apr 8, 2024
@pwalczysko
Copy link
Member

Screenshot 2024-04-12 at 14 24 57

Firefox, Mac M1 - ^^^ these sliders look strange, the left-hand side min slider

Copy link
Member

@pwalczysko pwalczysko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address the sliders issue in Preview panel. #477 (comment) and the spacing as per #477 (comment)

@pwalczysko
Copy link
Member

Screenshot 2024-04-12 at 14 38 05

Firefox, Mac, any image ^^^ Edit labels header is a bit squished into the box above it - this is different than without this PR, which is
Screenshot 2024-04-12 at 14 39 47

Could a proper spacing be added ?

@will-moore
Copy link
Member Author

@pwalczysko - thanks for the testing.

Slider handle positions (#477 (comment)) was actually caused by a different PR giving wrong values for min (-2147483648), so the slider minimum value was very low, causing the handles to overlap at the top. See ome/omero-web#536 (comment) (fixed now, but will need image panels to be removed or updated since they will still contain the wrong values).

The thick sliders in Firefox is fixed by b7cfdfa above and the Edit Labels spacing is also fixed now.

@pwalczysko
Copy link
Member

fixed now, but will need image panels to be removed or updated since they will still contain the wrong values).

You mean panels in pre-existing figures ? i.e. when a figure was created on merge-ci after the ome/omero-web#536 was open, but before your fix ome/omero-web#536 (comment), then the panels contain wrong values, do i get it right ? Panels updated or created before or after this period should contain correct values ?

@pwalczysko
Copy link
Member

The thick sliders in Firefox is fixed by b7cfdfa above and the Edit Labels spacing is also fixed now.

Confirming this fix.

Slider handle positions

Yes, these are fixed. Really not sure about the value remark #477 (comment)

Copy link
Member

@pwalczysko pwalczysko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please explain about #477 (comment). Thanks

@pwalczysko
Copy link
Member

pwalczysko commented Apr 17, 2024

Really cannot say that this is caused by this PR, but on merge-ci (ie. not for example on outreach), when I open a Figure it opens in a new tab (expected). But, only on merge-ci, this new tab is not marked with the OMERO logo.

merge-ci (webclient tab leftmost, OMERO.figure tabs to the right of it)
Screenshot 2024-04-17 at 19 01 04

outreach server (both webclient tab and figure tab to its right have OMERO logo):

Screenshot 2024-04-17 at 19 09 50

Edit: iviewer displays the same behaviour ^^^, probably ignore.

@will-moore
Copy link
Member Author

The favicon.ico is configured by the omero-web framework itself and not by the apps (iviewer/figure) at https://github.com/ome/omero-web/blob/02bd670ca7628ecb293108f394ef062a66a7beb4/omeroweb/settings.py#L867.
I'm not sure why it's not working on merge-ci but it's not related to this PR.

@will-moore
Copy link
Member Author

Re: #477 (comment) - as discussed, you are correct that this was a temporary bug caused by ome/omero-web#536 and won't affect any users.

@pwalczysko
Copy link
Member

Re: #477 (comment) - as discussed, you are correct that this was a temporary bug caused by ome/omero-web#536 and won't affect any users.

Understood, thank you.

@jburel
Copy link
Member

jburel commented Apr 29, 2024

Merging so we can move forward

@jburel jburel merged commit c288643 into ome:master Apr 29, 2024
1 check passed
@will-moore will-moore mentioned this pull request May 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants