Added Drag & Drop Reordering to Shopping List Card.#7296
Added Drag & Drop Reordering to Shopping List Card.#7296bramkragten merged 11 commits intohome-assistant:devfrom
Conversation
|
Also, you may need to rebase your PR again. Not sure about the build error you are getting. Something got messed up with the localize |
I've discussed the build error with balloob in #devs_frontend last night. Some localization issues on Lokalise had broken I fixed the localization issues on Lokalise, we should be good once it's pulled into repo today. After that I will push another commit to address your comment(s) which will re-trigger CI checks. I'm sure it will pass, otherwise I will rebase. EDIT: seems like balloob has already manually fixed |
|
I'm a little hesitant to always show the drag and drop handle for each row as it's not a common action. Maybe we can add a "reorder" menu item to turn on showing the handle, with a "finish" button ? |
|
He has an icon that does this |
|
Like Zack mentioned, this button toggles the drag & drop handles.
|
| 0, | ||
| this._uncheckedItems!.splice(evt.oldIndex, 1)[0] | ||
| ); | ||
| this._renderEmptySortable = true; |
There was a problem hiding this comment.
I think we should also do a round of _renderEmptySortable when this._uncheckedItems is changed? What happenes when you add an item while you are sorting the list? And what happens when storing the new order (line 329) failed?
There was a problem hiding this comment.
Tested the 2 scenarios without changing anything, both work fine.
When this._uncheckedItems is changed, the whole sortable div content will be re-rendered (at line 150), so everything should be straight.
There was a problem hiding this comment.
That is not true, Lit uses comments to keep track of things, because with sorting we change the DOM but not the comments Lit loses track of some elements. That's why we clear the DOM and render empty and start over. That can cause duplicate items in the list when not rendering empty.
There was a problem hiding this comment.
I'll do more digging tonight.
There was a problem hiding this comment.
@bramkragten I took another look, I still think there is no need to explicitly 'do a round of _renderEmptySortable when this._uncheckedItems is changed'.
Because at line 150: guard([this._uncheckedItems, this._renderEmptySortable]... decides that when either one of the 2 values (_uncheckedItems, _renderEmptySortable) changes, the content of sortable div will be re-rendered (doc).
When add an item while sorting the list, _uncheckedItems is changed therefore the content of sortable div will be re-rendered. Unless you are talking about the scenario that: the user adds an item after the user starts dragging and BEFORE the user puts it down, but I don't see how that is possible.
Also when storing the new order failed, _uncheckedItems is changed therefore the content of sortable div will be re-rendered.
In short: the list is messed up only after drag-n-drop, we do a round of _renderEmptySortable to re-render the list (also clean up the zombie elements). This is enough to keep the list always complying to Lit format.
In any other cases, the change of this._uncheckedItems will always re-render list, no need to use _renderEmptySortable, also there is no zombie elements generated by drag-n-drop.
There was a problem hiding this comment.
But while I was digging into it, I found another edge case bug:
- Drag an item around and put the item back to its original position.
- The Lit format is messed up by dragging around.
- But the cleanup block (toggling
_renderEmptySortable)won't be executed because putting the item back to its original position doesn't triggeronSortevent. - Therefore the format won't be cleaned up and causing issues in other user interactions.
The bug:
The fix I put in is that instead of observing onSort event, we observe onEnd event, which will be triggered even if an item is dragged and put back to its original position. In the block, we check on evt.oldIndex !== evt.newIndex to decide if a websocket message needs to be sent.
After fix:
There was a problem hiding this comment.
The problem is that re-rendering doesn't clear up the nodes that have no comment anymore.
There was a problem hiding this comment.
@bramkragten now I see what your concern is, and it's a very valid point.
My thinking is: the no-comment-nodes are only generated after drag&drop, so we only need to clear them up after drag&drop. We are already doing that in the sortable's onEnd event. That should guarantee we always clear up the no-comment-nodes.
If you still don't think it's safe, I have no problem adding the clear-up when this._uncheckedItems changes. Let me know.
BTW I need to work on the toast to show errors, I forgot it last time but now I remember.
There was a problem hiding this comment.
@bramkragten Thoughts? Does above make sense to you? Or should I go ahead add the clear-up when this._uncheckedItems changes? I don't mind either way.
| handle: "ha-svg-icon", | ||
| onSort: async (evt) => { | ||
| reorderItems(this.hass!, this._sortable.toArray()).catch(() => | ||
| this._fetchData() |
There was a problem hiding this comment.
We should let the user know it failed and why we undo all the sorting work.
There was a problem hiding this comment.
I will add a toast for the error message tonight.
|
Will work on addressing comments ASAP. |
|
Wanted to leave a comment that if there are issues in the future, please remind me, I'd be happy to help. |
6 participants
Proposed change
Add drag & drop reordering to shopping list card.
The Core PR: home-assistant/core#41585
Type of change
Example configuration
N/A
Additional information
Checklist
If user exposed functionality or configuration variables are added/changed: