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

Refactor Skeletontree tab as Antd <Tree /> component #7819

Merged
merged 64 commits into from
Aug 6, 2024
Merged

Conversation

hotzenklotz
Copy link
Member

@hotzenklotz hotzenklotz commented May 22, 2024

In an effort to use the same antd <Tree /> tree component for the skeletons, comments, and segments, I refactored the skeleton tab to use this component.

URL of deployed dev instance (used for testing):

Steps to test:

  • Do something outside the skeleton view so that it must update (select different tree, node, etc)
  • Rename skeletons
  • Drag and drop skeletons
  • Try context menu stuff
    • color changes
    • expand/collapse everything
  • Try really large skeleton
  • Try single select
  • Try multi select with CTRL/meta or SHIFT

TODOs:

  • Drag & Drop
  • Auto-Scrolling
  • Styling
  • Multi-Select
  • Search
  • Expand All / Expand Subgroup
  • Fix --- label on hover
  • Fix repeated SHIFT selection
  • Hide drag handler icon
  • Fix crash for "delete tree"
  • Empty groups should not show checkbox
  • Fix drag and drop moving too many trees
  • (Maybe) fix search behavior

Issues:


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

@hotzenklotz hotzenklotz mentioned this pull request May 27, 2024
11 tasks
@@ -41,15 +41,15 @@ import { getAdditionalCoordinatesAsString } from "oxalis/model/accessors/flycam_

const ALSO_DELETE_SEGMENT_FROM_LIST_KEY = "also-delete-segment-from-list";

export function ColoredDotIconForSegment({ segmentColorHSLA }: { segmentColorHSLA: Vector4 }) {
const hslaCss = hslaToCSS(segmentColorHSLA);
export function ColoredDotIcon({ colorRGBA }: { colorRGBA: Vector4 }) {
Copy link
Member Author

Choose a reason for hiding this comment

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

We can re-use this component for the skeleton, comments and segments tab. All three data types have RGB properties. So no need to use HSL instead.

@MichaelBuessemeyer
Copy link
Contributor

MichaelBuessemeyer commented Jun 5, 2024

Here is my pre-review testing feedback:

  • The tree items are too spacy:
    This is the current master version which does not line break the tree names and thus is horizontally scrollable but more dense
    image
    And this is your version:
    image
    I do prefer the dense (master) version

  • Rearranging trees within a group does no longer work on your branch.

  • Empty tree groups are opened by default on the master when:

  1. creating a new group and
  2. moving some trees into them.

On your branch one needs to expand the group as step 3.

  • On the master, the level intend seems to be smaller (due to the missing drag handle). Is it possible to make this a little narrower to make the view more compact?

These are the thing I noticed during testing. So far it looks very good beside this few points 🚀

Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer left a comment

Choose a reason for hiding this comment

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

Please just re-request a review once its time 🙏 :)

hotzenklotz and others added 11 commits July 5, 2024 17:57
* move skeleton expanded/collapsed group state to store

* WIP: expand and collapse all skeleton groups

* fix expand/collapse skeleton groups

* expand group if tree is dropped

* clean up code

* lint

* address review

* address review 2/2

* remove console.log

* lint

* add small condition for ondrop

* persist isExpanded state in backend

* make groups default expanded if field is absent in nml (backend only)

* make groups default expanded if for written nmls (backend only)

* fix backend treegroup update action tests

* add isExpanded to frontend nml support

* update snapshots

* omit object creation in var

* omit useless return statement in map

* add isExpanded prop to tree groups in backend testing fixtures (dummies)

* add test to backend nml test to test properly setting default to expanded

* add changelog

* WIP: address review

* fix bug where drag and drop of empty groups was failing

* remove comment

---------

Co-authored-by: Michael Büßemeyer <[email protected]>
Co-authored-by: Philipp Otto <[email protected]>
@philippotto
Copy link
Member

philippotto commented Aug 5, 2024

Really looking forward to see this merged! I tested this PR for a bit and found some things:

  • the scroll bar is not visible in dark mode (because the background color of the scroll bar is too dark?). in light mode, it works fine
  • when selecting a tree, it is always scrolled to be at the top of the view (maybe both points can be solved with scrollTo({..., align: "auto"})?
    • when manually selecting an item from the list, this is very annoying, because the tree item was perfectly visible, anyway.
    • when selecting a tree/node in the data viewport, this auto-scroll behavior also always enforces the item to be at the top of the view (if the list is long enough). however, when the item was visible anyway, it's not necessary to scroll at all.

won't fix?

  • if a tree name is very long, you cannot scroll horizontally. apparently, this is a limitation of antd. so probably a won't fix. the full name can be seen via the tooltip or the rename-field.

@philippotto
Copy link
Member

philippotto commented Aug 5, 2024

Hmm, I found another thing:

  • shift select often doesn't work. could it be that it performs the range by sorting alphabetically? however, the visible sorting might be by creation time. in the screenshot, the blue item was active and I shift-clicked on the gray item. nothing happens in that case

image

@MichaelBuessemeyer
Copy link
Contributor

Please wait until the backend changes of this pr have been reviewed (they haven't been reviewed yet. See: #7939 (comment))

@normanrz Could you take over looking over these changes? There shouldn't be many changes.

Copy link
Member

@normanrz normanrz left a comment

Choose a reason for hiding this comment

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

Backend looks 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.

awesome, works very well 👍

Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer left a comment

Choose a reason for hiding this comment

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

@hotzenklotz hotzenklotz enabled auto-merge (squash) August 6, 2024 11:57
@hotzenklotz hotzenklotz merged commit 9cc0635 into master Aug 6, 2024
2 checks passed
@hotzenklotz hotzenklotz deleted the tree-tree branch August 6, 2024 12:16
@@ -206,7 +205,6 @@
"react-redux": "^7.2.0",
"react-router-dom": "^5.1.2",
"react-sortable-hoc": "^2.0.0",
"react-sortable-tree": "^2.8.0",
Copy link
Member

Choose a reason for hiding this comment

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

The day has finally come 🎉

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.

Use antd Tree in skeleton view Shift select a range in tree tab
6 participants