787 allow users to adjust the order of label trees #1323
Conversation
…w-users-to-adjust-the-order-of-label-trees
This commit introduces sorting via drag-and-drop inside the label trees component. Sorting is stored in local storage and applied to all projects to which the volume is a part of.
This commit reintroduces up and down buttons for moving label trees. It also changes the collapse label tree functionality so that it will happen when clicking the label tree name.
…w-users-to-adjust-the-order-of-label-trees
…ttps://github.com/biigle/core into 787-allow-users-to-adjust-the-order-of-label-trees
|
Please link the issue to this PR by writing "Resolves #xxx" in the initial PR description or linking it manually in the "Development" section on the right. I did this for your last PRs but it's easier if you do it yourself 😉 |
There was a problem hiding this comment.
-
There are lots of coding style changes in here (e.g. changing
'to", rmoving trailing commas, adding spaces). Please revert all these. -
Please add the underline style of the label tree title and show the sorting buttons only when the user hovers over the title (not when hovering over the entire tree).
-
Please also add the sorting to the label trees tab of the project overview. -
You implemented the new behavior as opt-in in the component. I would choose always on instead (as the user has to actively change the order first before anything changes from the default).
You can still use the new property to disable the sorting for the "Favourites" label tree in the label trees component. Currently this is handled inside the label tree component but this component has nothing to to with controlling (or knowing about) the Favourites tree.
-
With some label tree names, the buttons can loose alignment and overflow to the bottom (see screenshot below). You should use a fixed
padding-rightfor the label tree title and wrap the buttons in an element withposition: absolute.
-
Please see the comment about linking the issue above.
Co-authored-by: Martin Zurowietz <[email protected]>
|
@dbrembilla scratch that part about the sorting in the project overview. It's too much effort for too little value. |
…ar on hover. Fixes styling for buttons so that they do not appear weird with long titles
…w-users-to-adjust-the-order-of-label-trees
|
I added the |
mzur
left a comment
mzur
left a comment
There was a problem hiding this comment.
I'll take a closer look at the components next time. First another round of higher-level comments:
-
Regarding your question: The padding should always be active. It's purpose is exactly to avoid any jumping content. In my test the padding still didn't seem enough:
-
I think we now need some visual indication that a label tree was collapsed. Before, this was shown with the collapse button icon but now there is no indication at all. What about showing the label tree title with the
text-mutedclass if it is collapsed? -
There seems to be an issue with favourite labels loaded from local storage. When I first add a favourite label, everything is ok. But when I reload the page, the favourite label tree shows up but the labels don't seem to have the "favourite" state. I can add them again and they are shown twice in the favourite tree. Demonstration:
Screencast.from.2025-12-12.15-40-59.webm
Co-authored-by: Martin Zurowietz <[email protected]>
…ttps://github.com/biigle/core into 787-allow-users-to-adjust-the-order-of-label-trees
This commit solves a nasty bug that basically caused weird behaviour in the `Favourites` labelTree, e.g. being unable to remove from favourites, the star not appearing and multiple favaourite assignment. This was caused by the fact that the sortedTrees property was assigned on `mounted`. This caused the `labelTree` component to be created after the `labelTrees` component was created, meaning that the `add-favourite` was emitted by the `labelTrees` component before the `labelTree` component was created. TThe handler for the `add-favourite` event was not yet set, causing the weird behaviour.
|
I tried to increase the padding to |
mzur
left a comment
mzur
left a comment
There was a problem hiding this comment.
I tweaked some of the hover logic of the label tree component as I realized that all this could be done with CSS instead (bee207c). I also made the up/down buttons a button group, which looks a little better.
I fumbled with the collapsed visual state a bit as I'm not 100% sure if the muted color without any other visual indicator is the best solution. But I also don't want to add another button/icon again. So I couldn't think of anything better for now.
More comments below. Except the things mentioned above I think the UI/UX is good. There is only a little refactoring potential in the code.
| let customOrderStorageKeys = []; | ||
| this.projectIds.forEach( | ||
| function (el) { | ||
| if (Number.isInteger(el)) { | ||
| customOrderStorageKeys.push(`biigle.label-trees.${el}.custom-order`) | ||
| } | ||
| } | ||
| ); | ||
| return customOrderStorageKeys; | ||
| }, |
There was a problem hiding this comment.
This could also be done with a map, since we should be able to trust that the IDs are integer:
return this.projectIds.map(id => `biigle.label-trees.${id}.custom-order`)There was a problem hiding this comment.
The key could also be confusing. What about:
`biigle.projects.${id}.label-trees.custom-order`| this.events.emit('clear'); | ||
| }, | ||
| handleAddFavourite(label) { | ||
| //TODO: here label.favourite does not get set to true. THIS MIGHT BE BECAUSE WE DONT USE A PROP ANYMORE TO CREATE LABELTREES |
| showMoveButtonUp() { | ||
| return this.treeIndex !== 0; | ||
| }, | ||
| showMoveButtonDown() { | ||
| return this.treeIndex !== this.maxTreeIndex; | ||
| }, |
There was a problem hiding this comment.
You could also control this in the label trees component, so each label tree just has showMoveButtonUp and showMoveButtonDown as prop. Then the individual tree would not need to know about the maxTreeIndex, for example. What do you think?
| emitMoveLabelTree(moveUp) { | ||
| let targetIdx = moveUp ? this.treeIndex - 1 : this.treeIndex + 1; | ||
| this.$emit( | ||
| 'move-label-trees', | ||
| this.treeIndex, | ||
| targetIdx, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Similar to the comment above, the tree could just emit a move-up or move-down event and it wouldn't need to know any of the actual logic of moving.
| changeCustomOrder() { | ||
| let customOrder = JSON.parse( | ||
| localStorage.getItem(this.customOrderStorageKey) | ||
| ); | ||
| if (customOrder) { | ||
| this.customOrder = customOrder; | ||
| } | ||
| } |
There was a problem hiding this comment.
This also seems to be used nowhere. Please check your changes for any other unused stuff.
| customOrder: { | ||
| immediate: true, | ||
| deep: true, | ||
| handler(customOrder) { | ||
| this.sortedTrees.sort(function (a, b) { | ||
| let idxa = customOrder.findIndex((val) => val == a.id); | ||
| let idxb = customOrder.findIndex((val) => val == b.id); | ||
| if (idxa && idxb) { | ||
| return idxa > idxb; | ||
| } else if (idxa) { | ||
| return true; | ||
| } else { | ||
| return false; | ||
| } | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
The "Vue way" would be to implement sortedTrees as computed property like this:
sortedTrees() {
return this.customOrder.map(id => this.trees.find(tree => id === tree.id));
}If there is no order from localStorage, you have to initialize customOrder in the created() hook like this:
this.customOrder = this.treeIds;Co-authored-by: Martin Zurowietz <[email protected]>
Co-authored-by: Martin Zurowietz <[email protected]>
…w-users-to-adjust-the-order-of-label-trees
This commit refactors labelTrees.vue and labelTree.vue. First, the information about whether the movement arrows in the labelTree component should be shown is a prop. Rather than emitting a generic move-label-trees property, a move-up and move-down events are defined. Then, in labelTrees, sortedTrees is now a computed property. The customOrderStorageKeys property is changed. The component's code was also adapted to the changes anticipated above. Finally, useless comments/code is removed.
Addresses #787 - Allows users to adjust the order of label trees