-
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] Drag within dimension group to reorder #80547
Conversation
411d2b7
to
65a8695
Compare
dfc2172
to
e9de46e
Compare
e9de46e
to
e7c0070
Compare
b56f8a3
to
3d6671b
Compare
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.
Visually I only saw one slight hiccup in the shifting of the dimensions where if you pull an item from the middle the margins are a bit off so that if you move to the top and drop it theres a slight moment where they then all shift down a couple pixels. Here's a movie: https://share.getcloudapp.com/JrugxKll
Do we now also get to remove the Group by
option in from the flyout?
Also, we need to figure out the keyboard accessibility of this interaction. I mentioned in Slack the EuiDragDrop has this out of the box, but if we can't use that here, we'll need a custom implementation.
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 in Chrome and Firefox and this works really slick now! Just left a small nit but otherwise I don't see any issues with the functionality.
54d2004
to
cac17f1
Compare
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 found a screenreader bug and left some comments. Overall still looking good, but I will want to do another test with screen reader before approving.
> | ||
<div className="lnsLayerPanel__dimension"> | ||
<NativeRenderer | ||
render={props.datasourceMap[datasourceId].renderDimensionTrigger} |
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!
4f4a1f4
to
878384e
Compare
@elasticmachine merge upstream |
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.
Hey, @mbondyra. Those focus changes we discussed look great! Well done.
I've left two small review comments for your attention. Additionally, I was wondering if it would be possible for you to change the size
props for the current EuiSpacer
components that appear before and after the .lnsLayerPanel__row
elements from s
to m
? Doing so will better match the provided designs.
Otherwise, this looks good from my perspective once these comments are addressed. Thanks so much!
x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/layer_panel.scss
Show resolved
Hide resolved
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.
Hey, @mbondyra. Thanks for making those changes. This looks great.
I noticed one last issue when dragging items within a dimension. The white background of the element being dragged doesn't appear to have a border-radius to match the rounded corners shown on the dashed border, as shown in the gif below. It's a super minor thing, but mentioning it in case it's a quick fix.
Aside from that, this looks good from my perspective. Approving.
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 again and it looks ready to me
💚 Build SucceededMetrics [docs]async chunks size
History
To update your PR or re-run it, just comment with: |
Summary
Fixes #67226
Works only for pie or table because of the bug in elastic-charts for xyChart (elastic/elastic-charts#868) - once the bug is fixed, it's one line change to make it work for xyCharts.
Here's the little demo:
Accessibility
Process for drag and drop with keyboard:
I've also added an
aria-described-by
element on the level of the group with description:Press space bar to start a drag. When dragging, use arrow keys to reorder. Press space bar again to finish.
and a
aria-live
element that has 3 states:Checklist