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

Synchronize list focus with active segment #7512

Merged
merged 24 commits into from
Jan 17, 2024

Conversation

dieknolle3333
Copy link
Contributor

@dieknolle3333 dieknolle3333 commented Dec 23, 2023

URL of deployed dev instance (used for testing):

  • https://___.webknossos.xyz

Steps to test:

  • create a lot of segments so that one can scroll in the segments view
  • click on a segment in the data viewport and ensure that the segments view scrolls and highlights to the corresponding item

TODOs:

  • create context menu entry
  • fix issue that only available segments are stored as selected (e.g. after deleting a segment, see commented-out-code). test this by removing a selected segment
  • highlight difference between jump to segment and Select segment in viewport segment dropdown
  • make sure the "click segment" action from context menu doesnt harm anything
  • actually focus segment in list, so scroll to it via this.tree.current.scrollTo({ key: selectedElement.key }); with the key being segment-<id>
    • fix that scrolling doesnt work for newly added segments
      -> may be bug
  • fix drag and drop in segment list, e.g. to groups (seperate issue: Support drag and drop of multiple segments #7527)
  • remove logging and read through the rest of the code

Issues:


(Please delete unneeded items, merge only when none are left open)

@dieknolle3333 dieknolle3333 self-assigned this Dec 23, 2023
@dieknolle3333
Copy link
Contributor Author

@philippotto I decided that a multiselection of segment should not be overwritten by a click on a segment. Because both the context menu entry and the segment click are causing the same action, the context menu entry doesnt overwrite it either. Thats probably counterintuitive. Obviously I could build around that, but I just wanted to make sure that we are keeping both ways to focus the segment :) So we are keeping the context menu entry and the click, right? Or should I ask for more opinions first?

@dieknolle3333
Copy link
Contributor Author

there may be edge cases where this context menu entry is displayed although it doesnt make sense, but I think that applies for more than this context menu entry. see #7538. I didnt know what edge exactly I found, but if its easy I will of course fix it in this PR :)

@dieknolle3333 dieknolle3333 marked this pull request as ready for review January 10, 2024 11:16
Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

great stuff :) I left some feedback, but overall it's already looking pretty good 👍

Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

Works great for me :)

@dieknolle3333 dieknolle3333 enabled auto-merge (squash) January 17, 2024 09:06
@dieknolle3333 dieknolle3333 merged commit 8c1858d into master Jan 17, 2024
2 checks passed
@dieknolle3333 dieknolle3333 deleted the synchronize-list-focus-with-active-segment branch January 17, 2024 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Segment list focus is not in sync with active segment
3 participants