-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Lens] Add new drag and drop capabilities #89745
[Lens] Add new drag and drop capabilities #89745
Conversation
@elasticmachine merge upstream |
d7c03a8
to
d933737
Compare
@elasticmachine merge upstream |
d933737
to
476abce
Compare
eb8984b
to
9b07acd
Compare
Overall: this is awesome! 🎉 This is probably one of our most complicated intersections of accessibility and functionality and y'all are just nailing it. Before I dive in, I just want to give some background on how I'm testing right now:
Disclaimer before I jump in: this is a super complicated series of interaction. If you're at all unsure about what I mean, disagree, have a better idea, etc PLEASE PLEASE PLEASE reach out and let's talk about it. A lot of these are rough suggestions to move things in the right direction, and nothing here is final or cut-and-dry. A few early thoughts:
<At this point I stopped reviewing but will do more this week/as this PR progresses> |
4e60e2f
to
9349f89
Compare
Hi @myasonik, thank you for your feedback! here's my responses:
|
@mbondyra: Wow, this is some really incredible work! Very well done! I've been playing around with it in Chrome for a bit and it's wonderful. I've got a handful of notes below regarding some aspects of the UI/UX. Please forgive any repetition with @myasonik's feedback.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is very close to being ready! I tested as many of the interactions as I could in Firefox. I would be comfortable merging and iterating on this feature once the level of test coverage includes the new scenarios.
@MichaelMarcialis I agree that we should implement your feedback except the one about the ghost image. That's a feature of the web browser which we would need to reimplement.
x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/droppable.ts
Outdated
Show resolved
Hide resolved
@elasticmachine merge upstream |
Thank you all for feedback and kind words! I've implemented all corrections from @MichaelMarcialis except for the ghost image – I'm all for doing it, but in the second iteration as it's a bigger effort and I would like to merge this PR in 7.12, if acceptable by everyone. Also, please check how not blocking the right/left arrows while reordering feels like right now. Regarding the list from Michail, we spoke about it offline and the only thing I don't see ready yet in this PR is focus management – sometimes focus gets lost (when element is being moved to other dimension and removed from the old one) – I'll submit another PR for it next week! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also leaving a few small CSS comments. Thanks!
Sounds good. Did you want me to open a new issue for that? Or have you already done so? |
Yes, please @MichaelMarcialis , if you could open an issue that would be great 🙏 |
Co-authored-by: Michael Marcialis <michael@marcial.is>
Co-authored-by: Michael Marcialis <michael@marcial.is>
…bondyra/kibana into lens/dnd_add_copy_to_dimension
Did some more testing of this both as a keyboard user and a screen reader user and this is working so nicely. Feels great to use, I love it. The focus drop is the one thing that I could find and you already know about that. Otherwise, such a smooth experience.
I'm definitely a fan. Feels way more natural for me while exploring. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making those changes, @mbondyra! LGTM!
Also, I've opened a separate issue for the request to have keyboard-selected items follow user traversal of drop zones (and accompanying phantom space request).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested and didn't find any problems, this LGTM. There are a bunch of calls to getOperationSupportMatrix
we cannmake less expensive because we only need the operations for a single field, but we are building the matrix for the whole index pattern. This will be especially relevant for index patterns with tons of fields.
What I had in mind is to pass in an optional field name and if that's set, the expensive code in getAvailableOperationsByMetadata
doesn't iterate over all of the fields but just the required one to build the part of the matrix we actually require.
Not a blocker though
const layerIndexPatternId = props.state.layers[props.layerId].indexPatternId; | ||
|
||
function hasOperationForField(field: IndexPatternField) { | ||
return Boolean(operationSupportMatrix.operationByField[field.name]); | ||
return !!getOperationSupportMatrix(props).operationByField[field.name]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Smart way of avoiding a call to getOperationSupportMatrix
!
}; | ||
|
||
const newColumnOrder = [...layer.columnOrder]; | ||
// put a new bucketed dimension just in front of the metric dimensions, a metric dimension in the back of the array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: I'm working on this on the column transpose PR
...s, | ||
dropTargetsByOrder: { | ||
...s.dropTargetsByOrder, | ||
[order.join(',')]: dropTarget ? dropTarget : undefined, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: why the ternary here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Tested on Chrome & Safari ✅
Really great stuff @mbondyra , exceptional work! |
💚 Build SucceededMetrics [docs]Module Count
Async chunks
History
To update your PR or re-run it, just comment with: |
Created interactions
Fixes #79266
Solves Partially #51739
Field keyboard interactions ( drop to workspace, existing field and empty field)

Select a field with enter and then move between groups with arrows left/right.
Inside group interactions (reorder and duplicate)

Reorder stays as before - use arrows down/up to move element.
Duplicate - choose the new dimension button as a target to duplicate element. (navigate with arrows left/right)
Between compatible group interactions (move and replace) (navigate with arrows left/right)

Between incompatible groups (convert & replace and convert & move) (navigate with arrows left/right)

Summary
created announcement system
added order and dropsByTarget that are registering when dragging element changes
replaced canHandleDrop to getDropTypes that gives more information and will be useful when we have multiple drops
types
styles for datapanel source were changed
extending
value
to also holdhumanData
needed for accessibilityalong with
getClassesOnEnter
addedgetClassesOnDropable
that is run when element is droppable for the current dragdocumentation
tests
functional tests
destructuring props
documentation in README.md