[Lens] Refactor reorder drag and drop #88578
Conversation
|
@elasticmachine merge upstream |
mbondyra
force-pushed
the
pr/86901
branch
2 times, most recently
from
2d605f3 to
543e481
Compare
# Conflicts: # x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/layer_panel.tsx # x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.test.tsx # x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx # x-pack/plugins/lens/public/indexpattern_datasource/field_item.test.tsx # x-pack/plugins/lens/public/indexpattern_datasource/field_item.tsx # x-pack/plugins/lens/public/indexpattern_datasource/field_list.tsx # x-pack/plugins/lens/public/indexpattern_datasource/fields_accordion.test.tsx # x-pack/plugins/lens/public/indexpattern_datasource/fields_accordion.tsx
|
@elasticmachine merge upstream |
| } | ||
| setActiveDropTarget(undefined); | ||
| setDragging(undefined); | ||
| setKeyboardMode(false); |
There was a problem hiding this comment.
It looks like you've copy+pasted the logic that happens on dragEnd into the logic for drop, because in both cases the drop clears itself. Can you call dragEnd() from the drop function to reduce duplication?
There was a problem hiding this comment.
actually, I can safely remove it as dragEnd is always run after drop. EDIT: Wrong, I cannot - dragEnd doesn't exist in droppable elements, just in dragInners.
| reorderableGroup, | ||
| onDrop, | ||
| setA11yMessage, | ||
| } = props; |
There was a problem hiding this comment.
This style of spread operator is kind of hard to maintain because we could easily add a top-level prop and it wouldn't be passed to the reordering code. Do we need the spread operator at all? If we do, maybe you can do something like: const { unused1, unused2, ...used } = props and exclude only the props you don't need?
There was a problem hiding this comment.
hmmm, is this comment is on the right line? I don't see any spread operator close 🤔 Could you please take a look? EDIT: Ok, now I got what you mean, destructuring the props object - but I do that to use them then in the code. I still pass original props object to DragInner. Maybe I don't understand completely what you mean, could give me a concrete code example? 🙏
There was a problem hiding this comment.
Yes I meant destructuring! Like I said this is more of a stylistic comment, because sometimes we destructure too many properties. For example, I would want to avoid this style of code:
const { a, b, c, d, e, f, g } = props;
if (a) {
...
}
return anotherFunction({ a, b, c, d });
In that case, do we really need to destructure b, c, and d? What about:
const { a, e, f, g, ...functionProps } = props;
if (a) {
...
}
return anotherFunction({ a, ...functionProps });
But you do not need to agree with me here, it's something I'm starting to notice in the Lens code but I'm not sure this is the right fix.
There was a problem hiding this comment.
Ahhh, the famous apropcalypse... Yeah, I also notice that and we might want to address it at some point but I feel like in this case the destructured props are thrown in so many different places that it wouldn't make sense. I'll take a deeper look.
| dropped: (position: number, prevPosition: number) => | ||
| i18n.translate('xpack.lens.dragDrop.dropMessageReorder', { | ||
| defaultMessage: | ||
| 'You have dropped the item. You have moved the item from position {prevPosition} to positon {position}', |
There was a problem hiding this comment.
No indication of the itemLabel of the dropped item?
x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/config_panel.tsx
Show resolved
Hide resolved
...lugins/lens/public/editor_frame_service/editor_frame/config_panel/empty_dimension_button.tsx
Outdated
Show resolved
Hide resolved
| activeVisualization, | ||
| updateVisualization, | ||
| updateDatasource, | ||
| } = props; |
There was a problem hiding this comment.
Same comment about spreading so many variables vs excluding ones you don't need
|
@elasticmachine merge upstream |
x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/dimension_button.tsx
Outdated
Show resolved
Hide resolved
|
@elasticmachine merge upstream |
|
@wylieconlon I'll address three leftovers from your review in the next PR to close it and avoid conflicts. It's just those stylistic things:
For now I am merging this one once CI passes. |
💚 Build SucceededMetrics [docs]Module Count
Async chunks
History
To update your PR or re-run it, just comment with: |
Summary
This PR changes the keyboard reorder to not apply automatic changes on arrow_down/up but to confirm with enter (and blur/cancel changes with escape). It also involves some refactoring changes to add new and functionalities in the future.
Technical details changes:
dropToandonDropfunctions are merged into function that takes 2 params:onDrop(dragging, target)An internal state of
DragDropInnerelement was removed and replaced with globalactiveDropTargetelement.To make things easier and more performant,
DragDropcomponent internal implementation divides the elements intoDragInner,ReorderableDrag(which "decorates" DragInner),DropInnerandReorderableDrop(decorates DropInner).prop
valuewith required unique propertyidbecomes required for allDragDropcomponents. This eliminates the need forisValueEqualas the objects can be compared by id.some tests are missing