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

Label sorting with drag and drop #575

Merged
merged 5 commits into from
Jul 11, 2024
Merged

Label sorting with drag and drop #575

merged 5 commits into from
Jul 11, 2024

Conversation

Tom-TBT
Copy link
Contributor

@Tom-TBT Tom-TBT commented Jul 3, 2024

Hello Will,
here's a PR to sort labels with a drag-and-drop. The cursor changes when hovering the labels, and moving the labels up or down, the order of the labels will be updated.

For the tests I conducted it works well. This handles:

  • multiple panels (all labels equal, all different, mix of both)
  • multiple label position (one set of positions is updated at a time)

This is the work from my new student Tehmina who is helping me with changes I'd like to make. @MinaEnayat

}

// Destroy existing Sortable instances if any
if (self.sortables) {
Copy link
Member

Choose a reason for hiding this comment

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

PR looks great and works well!

The only question I have is about the destroy() here...
If I create a sortable, then select a different panel in the figure then this SelectedPanelsLabelsView gets lost without destroying the selectable(s) that were created.
I don't know if that really matters - probably not. Does it hurt if we don't clear destroy them? E.g. use up memory etc??
If we do want to make sure they all get destroyed, I wonder if a more global list of sortables (instead of just this component) or possibly try to handle the removal of the component and destroy them there. E.g. see

clear: function() {
(which was originally created to handle the destruction of jQuery sliders - now I don't use jQuery sliders, so it does less now).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I followed the clear example and now destroy the sortables when the panel is deselected. Thanks for spotting that

Copy link
Member

@will-moore will-moore left a comment

Choose a reason for hiding this comment

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

Thanks for the fix - testing locally looks good and functional testing on merge-ci is behaving as expected 👍

@will-moore will-moore merged commit fefe87c into ome:master Jul 11, 2024
1 check passed
@will-moore
Copy link
Member

This is now released in omero-figure 7.1.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants