Skip to content

[Lens] Drag within dimension group to reorder#80547

Merged
mbondyra merged 51 commits intoelastic:masterfrom
mbondyra:lens/dnd_to_reorder
Nov 3, 2020
Merged

[Lens] Drag within dimension group to reorder#80547
mbondyra merged 51 commits intoelastic:masterfrom
mbondyra:lens/dnd_to_reorder

Conversation

@mbondyra
Copy link
Copy Markdown
Contributor

@mbondyra mbondyra commented Oct 14, 2020 •
edited
Loading

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:

gif

Accessibility

Process for drag and drop with keyboard:

  1. To enter keyboard mode, user has to select the first of three focus points in the button (1 -> drag and drop controller, second opens the flyout, third is a delete button).
  2. Then press space bar / enter to start a drag - the buttons in the group will turn green.
  3. Use arrows up/down to move items – the changes are applied immediately to the chart
  4. Escape dragging mode with pressing space/enter/escape.

accessibility

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:

  1. "" (empty) when drag is inactive.
  2. "You have lifted an item in position {position}" when element has been lifted but no changes happened yet.
  3. "You have moved the item from position {prevPosition} to position {position}" when element has just been repositioned

Checklist

  • accessibility check
  • tests for keyboard behavior
  • To avoid conflicts, this branch is build on top of [Lens] Split up dimension panel code #80423 (will be rebased once we merge the above)
  • Documentation
  • Unit or functional tests were updated or added to match the most common scenarios
  • Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility) – the reorder functionality drag and drop is not keyboard usable, but we have another way of achieving the same effect (dropdown list on 'group by' setting)
  • Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
  • cross browser check (Safari)

@mbondyra mbondyra force-pushed the lens/dnd_to_reorder branch 3 times, most recently from 411d2b7 to 65a8695 Compare October 16, 2020 13:40
@mbondyra mbondyra added v7.11.0 v8.0.0 enhancement New value added to drive a business result Feature:Lens Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// labels Oct 16, 2020
@mbondyra mbondyra changed the title [Lens] drag and drop to reorder [Lens] Drag within dimension group to reorder Oct 16, 2020
@mbondyra mbondyra force-pushed the lens/dnd_to_reorder branch 6 times, most recently from dfc2172 to e9de46e Compare October 19, 2020 15:05
@elastic elastic deleted a comment from kibanamachine Oct 19, 2020
@mbondyra mbondyra force-pushed the lens/dnd_to_reorder branch from e9de46e to e7c0070 Compare October 19, 2020 15:31
@mbondyra mbondyra force-pushed the lens/dnd_to_reorder branch from b56f8a3 to 3d6671b Compare October 19, 2020 18:38
@mbondyra mbondyra requested a review from cchaos October 19, 2020 18:38
@elastic elastic deleted a comment from kibanamachine Oct 19, 2020
cchaos
cchaos reviewed Oct 19, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@cchaos cchaos left a 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.

@mbondyra mbondyra marked this pull request as ready for review October 20, 2020 07:37
@mbondyra mbondyra requested a review from a team October 20, 2020 07:37
@mbondyra mbondyra requested a review from a team as a code owner October 20, 2020 07:37
flash1293
flash1293 approved these changes Oct 30, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@flash1293 flash1293 left a 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.

@mbondyra mbondyra force-pushed the lens/dnd_to_reorder branch from 54d2004 to cac17f1 Compare October 30, 2020 12:47
wylieconlon
wylieconlon reviewed Oct 30, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@wylieconlon wylieconlon left a 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.

x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/layer_panel.tsx
>
<div className="lnsLayerPanel__dimension">
<NativeRenderer
render={props.datasourceMap[datasourceId].renderDimensionTrigger}
Copy link
Copy Markdown
Contributor

@wylieconlon wylieconlon Oct 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@mbondyra mbondyra force-pushed the lens/dnd_to_reorder branch from 4f4a1f4 to 878384e Compare November 2, 2020 12:50
@mbondyra
Copy link
Copy Markdown
Contributor Author

mbondyra commented Nov 2, 2020

@elasticmachine merge upstream

MichaelMarcialis
MichaelMarcialis requested changes Nov 3, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@MichaelMarcialis MichaelMarcialis left a 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!

@mbondyra mbondyra mentioned this pull request Nov 3, 2020
Closed
MichaelMarcialis
MichaelMarcialis approved these changes Nov 3, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@MichaelMarcialis MichaelMarcialis left a 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.

drag

Aside from that, this looks good from my perspective. Approving.

wylieconlon
wylieconlon approved these changes Nov 3, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@wylieconlon wylieconlon left a 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

@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Nov 3, 2020

💚 Build Succeeded

Metrics [docs]

async chunks size

id before after diff
lens 1.0MB 1.0MB +16.4KB

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@mbondyra mbondyra merged commit 6689167 into elastic:master Nov 3, 2020
@mbondyra mbondyra deleted the lens/dnd_to_reorder branch November 3, 2020 20:18
mbondyra added a commit to mbondyra/kibana that referenced this pull request Nov 3, 2020
@mbondyra
[Lens] Drag within dimension group to reorder (elastic#80547)
71000c4
@mbondyra mbondyra mentioned this pull request Nov 3, 2020
Merged
mbondyra added a commit that referenced this pull request Nov 4, 2020
@mbondyra
[Lens] Drag within dimension group to reorder (#80547) (#82500)
2103911
@MichaelMarcialis MichaelMarcialis mentioned this pull request Nov 10, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@flash1293 flash1293 flash1293 approved these changes

@MichaelMarcialis MichaelMarcialis MichaelMarcialis approved these changes

@cchaos cchaos Awaiting requested review from cchaos

@dej611 dej611 Awaiting requested review from dej611

+1 more reviewer

@wylieconlon wylieconlon wylieconlon approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Feature:Lens release_note:enhancement Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// v7.11.0 v8.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Lens] Drag within dimension group to reorder

8 participants

@mbondyra @elasticmachine @flash1293 @kibanamachine @cchaos @wylieconlon @dej611 @MichaelMarcialis