[Lens] Keyboard-selected items follow user traversal of drop zones #90546

mbondyra · 2021-02-06T10:03:49Z

Summary

Fixes #90339

Interaction with field:

Interaction between dimensions

Interaction when reordering

Design Questions:

Should we leave the focus ring in the original place of the dragging element? I don't do it here, but maybe I should.
When user selects the workspace as an active drop, we don't display the ghost image at all, only for dimension elements. Is this ok?
A field from datasource panel is copied and not moved, so I decided not to hide it. Let me know your thoughts :)
Maybe for the duplicate in group interaction (and other interactions that will come later on where we don't move the element but copy it) we shouldn't hide the dragging field?

elasticmachine · 2021-02-06T10:03:50Z

Pinging @elastic/kibana-app (Team:KibanaApp)

mbondyra · 2021-02-08T10:41:55Z

@elasticmachine merge upstream

myasonik · 2021-02-08T21:44:59Z

x-pack/plugins/lens/public/drag_drop/drag_drop.tsx

+  /**
+   * don't display ghost image for the drop element
+   */
+  noGhost?: boolean;


I find negative props somewhat confusing in components. Maybe others do to?

Especially when passing in false you end up with a double negative when thinking it through and that can throw people (me, at least) for a loop.

I understand and it makes sense. However, on the other side, this construction allows us to treat displaying a ghost as a default behaviour without passing extra parameter and that's what I was trying to do here. So for most components, I can do <DragDrop draggable .../> and I don't have to add ghost = {true} (or specify a default value in the component). I understand it's a bit on the edge when it comes to readability so I'd like to know the opinion of another person reviewing this 🙏

myasonik

Looks good! I like the effect and when testing with Voice Over I didn't notice anything breaking.

Just one small bug (below) and a small nit/question in the code.

Kind of an odd bug, and might be a browser bug, only in Safari in full screen mode, if I'm moving a dimension to a different drop target but then cancel by pressing esc, Safari exists full screen mode. This doesn't happen in other browsers. And this doesn't happen if I'm reordering and press esc. Maybe an extra event.preventDefault() somewhere can prevent this?

mbondyra · 2021-02-09T09:18:35Z

Thanks for the review @myasonik! I fixed the bug with your suggestion, e.stopPropagation()

flash1293 · 2021-02-09T15:03:21Z

@elasticmachine merge upstream

…ation

flash1293 · 2021-02-09T16:02:35Z

These failures don't seem related, testing again.

flash1293 · 2021-02-09T16:02:44Z

@elasticmachine merge upstream

…ation

flash1293

The feature LGTM once the PR is green, this is a smart way to implement it. Tested in Chrome and Firefox and it renders fine

I think it's worth adding a unit test to the drag_drop tests to verify whether the ghost image is put correctly into the context and rendered correctly on the active target.

Left a comment about the prop name, but it's not a big deal IMHO.

flash1293 · 2021-02-09T16:16:21Z

x-pack/plugins/lens/public/drag_drop/drag_drop.tsx

+  /**
+   * don't display ghost image for the drop element
+   */
+  noGhost?: boolean;


I don't have a strong feeling here, but we could call the prop ghost and treat undefined as true:

{(ghost || typeof ghost === 'undefined') && dragging?.ghost && activeDropTarg...

flash1293 · 2021-02-10T12:38:27Z

@elasticmachine merge upstream

…ation

MichaelMarcialis · 2021-02-10T15:52:55Z

This looks awesome, @mbondyra! Here are my responses to your questions.

Should we leave the focus ring in the original place of the dragging element? I don't do it here, but maybe I should.

No, I think leaving behind an empty focus ring in the original location when changing dimensions would end up being a distraction. If anything, it probably makes the most sense to keep the focus ring around the ghosted item to make it very apparent where the item is moving. Here's an example below.

When user selects the workspace as an active drop, we don't display the ghost image at all, only for dimension elements. Is this ok?

Ideally, I'd love to see the ghost behavior be consistent, even when dropping a field on the workspace. Is it possible to add the ghosted field to the top right of the workspace when it is the currently selected drop zone?

A field from datasource panel is copied and not moved, so I decided not to hide it. Let me know your thoughts :)

Correct. Since the field isn't being removed from that list, I think it makes sense not to hide it.

Maybe for the duplicate in group interaction (and other interactions that will come later on where we don't move the element but copy it) we shouldn't hide the dragging field?

I'm OK leaving it as is for the moment. It might be more confusing on the user's part to see the ghost in the "add" drop zone but then also seeing the item in its original spot simultaneously.

Otherwise, here's some additional comments from my review:

Would it be possible to hide the tooltip icon for the ghost version of fields being actively dragged?

The ghost vertical offsets for the "add" drop zones and the "replace" drop zones appear to be different. Can these both be a consistent 8px vertical offset?

There is a double border occurring in the workspace when in drag mode (keyboard and mouse). One border is the panel and the other is the drop zone. Is it possible to just stylize the panel border during a drag event rather than adding a new border inside the panel?

flash1293 · 2021-02-11T10:04:35Z

@elasticmachine merge upstream

…ation

mbondyra · 2021-02-11T14:02:19Z

Hey @MichaelMarcialis I've corrected all the things you mentioned except for the workspace double border - everytime I am applying it in some way, some other thing breaks and I want to merge this branch before FF on Tuesday. Are you ok with merging this as it is and then correcting the double border in separate PR? 🙏

…_add_ghost_image_to_keyboard_navigation

wylieconlon

LGTM, tested in Firefox!

wylieconlon · 2021-02-11T16:03:48Z

x-pack/plugins/lens/public/drag_drop/drag_drop.tsx

@@ -416,8 +446,13 @@ const DropInner = memo(function DropInner(props: DropInnerProps) {
    setActiveDropTarget(undefined);
    setKeyboardMode(false);
  };
+
+  // const shouldShowGhost =  isActiveDropTarget && dropType !== 'reorder';


Commented code

wylieconlon

Sorry, I found a bug caused by this PR: the entire workspace panel is empty in Firefox.

wylieconlon · 2021-02-11T16:27:28Z

...s/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel_wrapper.scss

@@ -34,6 +33,8 @@
  // Color the whole panel instead
  background-color: transparent !important; // sass-lint:disable-line no-important
  border: none !important; // sass-lint:disable-line no-important
+  width: 100%;
+  height: 100%;


I don't see why this behavior changed, can you explain?

Yes, so we have this style:

.lnsWorkspacePanelWrapper .lnsWorkspacePanelWrapper__pageContentBody > * { flex: 1 1 100%; display: flex; align-items: center; justify-content: center; overflow: hidden;

that before was applied directly to .lnsWorkspace. Now, when I added .lnsDragDrop__container as a parent of the element, .lnsDragDrop__container got the styles above so I needed to ensure the child (.lnsWorkspace) was "spread" inside.

MichaelMarcialis

Thanks for making those changes, @mbondyra. Let me know if you want me to create that separate issue for the double border.

I left one small suggestion. Otherwise, this looks good to me. Approving now so I don't hold you up further.

x-pack/plugins/lens/public/drag_drop/drag_drop.scss

Co-authored-by: Michael Marcialis <[email protected]>

mbondyra · 2021-02-15T09:33:32Z

@elasticmachine merge upstream

…ation

kibanamachine · 2021-02-15T11:42:04Z

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`lens`	892.3KB	896.4KB	+4.1KB

History

💚 Build #106194 succeeded 2cef3ea
💚 Build #106090 succeeded 6f60e55
💔 Build #106058 failed 7e2d40d
💔 Build #105223 failed 464f71c
💔 Build #105189 failed 97c3393

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

mbondyra · 2021-02-15T11:47:57Z

@wylieconlon I want to merge this one before FF so I'lll merge it despite your 'changes requested' - I addressed your request already, hope it's fine!

…lastic#90546)

…90546) (#91383)

…ndition-for-hiding-recommded-allocation * 'master' of github.com:elastic/kibana: [Discover] Fix toggling multi fields from doc view table (elastic#91121) [ML] Data Frame Analytics: ROC Curve Chart (elastic#89991) skip flaky suite (elastic#86948) skip flaky suite (elastic#91191) Fix date histogram time zone for rollup index (elastic#90632) [Search Source] Fix retrieval of unmapped fields; Add field filters (elastic#89837) [Logs UI] Use useMlHref hook for ML links (elastic#90935) Fix values of `products.min_price` field in Kibana sample ecommerce data set (elastic#90428) [APM] Darker shade for Error group details labels (elastic#91349) [Lens] Adjust new copy for 7.12 (elastic#90413) [ML] Unskip test. Fix modelMemoryLimit value. (elastic#91280) [Lens] Fix empty display name issue in XY chart (elastic#91132) [Lens] Improves error messages when in Dashboard (elastic#90668) [Lens] Keyboard-selected items follow user traversal of drop zones (elastic#90546) [Lens] Improves ranking feature in Top values (elastic#90749) [ILM] Rollover min age tooltip and copy fixes (elastic#91110) # Conflicts: # x-pack/plugins/index_lifecycle_management/__jest__/client_integration/edit_policy/edit_policy.test.ts

mbondyra added Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:Lens v7.12.0 labels Feb 6, 2021

mbondyra requested a review from a team February 6, 2021 10:03

mbondyra requested a review from a team as a code owner February 6, 2021 10:03

mbondyra changed the title ~~adding a ghost image~~ [Lens] Keyboard-selected items follow user traversal of drop zones Feb 6, 2021

mbondyra force-pushed the lens/dnd_add_ghost_image_to_keyboard_navigation branch 4 times, most recently from 763dc90 to 20b5f37 Compare February 7, 2021 17:06

mbondyra requested review from myasonik and MichaelMarcialis February 8, 2021 16:56

myasonik reviewed Feb 8, 2021

View reviewed changes

myasonik approved these changes Feb 8, 2021

View reviewed changes

adding a ghost image

e19f9ad

mbondyra force-pushed the lens/dnd_add_ghost_image_to_keyboard_navigation branch from b085a7d to e19f9ad Compare February 9, 2021 08:48

fix safari fullscreen

a2f1c14

Merge branch 'master' into lens/dnd_add_ghost_image_to_keyboard_navig…

97c3393

…ation

Merge branch 'master' into lens/dnd_add_ghost_image_to_keyboard_navig…

464f71c

…ation

flash1293 approved these changes Feb 9, 2021

View reviewed changes

mbondyra and others added 2 commits February 10, 2021 16:29

show ghost instead of noGhost

45dbf89

Merge branch 'master' into lens/dnd_add_ghost_image_to_keyboard_navig…

1c2d223

…ation

kibanamachine and others added 2 commits February 11, 2021 05:04

Merge branch 'master' into lens/dnd_add_ghost_image_to_keyboard_navig…

82c7c22

…ation

cr corrections

c13cfd0

mbondyra added 2 commits February 11, 2021 15:46

Merge commit '6bd0a7fcc5f477b75ca173ee0de11ebcd2898f4f' into lens/dnd…

a84ed51

…_add_ghost_image_to_keyboard_navigation

removing commented code

7e2d40d

wylieconlon approved these changes Feb 11, 2021

View reviewed changes

remvoing commented code

3eb52e6

wylieconlon suggested changes Feb 11, 2021

View reviewed changes

remvoing commented code

6f60e55

elastic deleted a comment from kibanamachine Feb 11, 2021

mbondyra requested a review from wylieconlon February 11, 2021 16:19

wylieconlon reviewed Feb 11, 2021

View reviewed changes

MichaelMarcialis approved these changes Feb 11, 2021

View reviewed changes

x-pack/plugins/lens/public/drag_drop/drag_drop.scss Outdated Show resolved Hide resolved

Update x-pack/plugins/lens/public/drag_drop/drag_drop.scss

2cef3ea

Co-authored-by: Michael Marcialis <[email protected]>

Merge branch 'master' into lens/dnd_add_ghost_image_to_keyboard_navig…

1f6aa1a

…ation

mbondyra merged commit 0ee7be1 into elastic:master Feb 15, 2021

mbondyra mentioned this pull request Feb 15, 2021

[7.x] [Lens] Keyboard-selected items follow user traversal of drop zones (#90546) #91383

Merged

mbondyra added a commit to mbondyra/kibana that referenced this pull request Feb 15, 2021

[Lens] Keyboard-selected items follow user traversal of drop zones (e…

061cef6

…lastic#90546)

mbondyra deleted the lens/dnd_add_ghost_image_to_keyboard_navigation branch February 15, 2021 12:03

mbondyra added a commit that referenced this pull request Feb 15, 2021

[Lens] Keyboard-selected items follow user traversal of drop zones (#…

89c7d02

…90546) (#91383)

mbondyra mentioned this pull request Feb 19, 2021

[Lens] Fix overlowing content on a chart for charts and table #92006

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Lens] Keyboard-selected items follow user traversal of drop zones #90546

[Lens] Keyboard-selected items follow user traversal of drop zones #90546

mbondyra commented Feb 6, 2021 •

edited

Loading

elasticmachine commented Feb 6, 2021

mbondyra commented Feb 8, 2021

myasonik Feb 8, 2021

mbondyra Feb 9, 2021 •

edited

Loading

myasonik left a comment

mbondyra commented Feb 9, 2021

flash1293 commented Feb 9, 2021

flash1293 commented Feb 9, 2021

flash1293 commented Feb 9, 2021

flash1293 left a comment •

edited

Loading

flash1293 Feb 9, 2021

flash1293 commented Feb 10, 2021

MichaelMarcialis commented Feb 10, 2021

flash1293 commented Feb 11, 2021

mbondyra commented Feb 11, 2021

wylieconlon left a comment

wylieconlon Feb 11, 2021

wylieconlon left a comment

wylieconlon Feb 11, 2021

mbondyra Feb 11, 2021 •

edited

Loading

MichaelMarcialis left a comment

mbondyra commented Feb 15, 2021

kibanamachine commented Feb 15, 2021

mbondyra commented Feb 15, 2021

[Lens] Keyboard-selected items follow user traversal of drop zones #90546

[Lens] Keyboard-selected items follow user traversal of drop zones #90546

Conversation

mbondyra commented Feb 6, 2021 • edited Loading

Summary

elasticmachine commented Feb 6, 2021

mbondyra commented Feb 8, 2021

myasonik Feb 8, 2021

Choose a reason for hiding this comment

mbondyra Feb 9, 2021 • edited Loading

Choose a reason for hiding this comment

myasonik left a comment

Choose a reason for hiding this comment

mbondyra commented Feb 9, 2021

flash1293 commented Feb 9, 2021

flash1293 commented Feb 9, 2021

flash1293 commented Feb 9, 2021

flash1293 left a comment • edited Loading

Choose a reason for hiding this comment

flash1293 Feb 9, 2021

Choose a reason for hiding this comment

flash1293 commented Feb 10, 2021

MichaelMarcialis commented Feb 10, 2021

flash1293 commented Feb 11, 2021

mbondyra commented Feb 11, 2021

wylieconlon left a comment

Choose a reason for hiding this comment

wylieconlon Feb 11, 2021

Choose a reason for hiding this comment

wylieconlon left a comment

Choose a reason for hiding this comment

wylieconlon Feb 11, 2021

Choose a reason for hiding this comment

mbondyra Feb 11, 2021 • edited Loading

Choose a reason for hiding this comment

MichaelMarcialis left a comment

Choose a reason for hiding this comment

mbondyra commented Feb 15, 2021

kibanamachine commented Feb 15, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

History

mbondyra commented Feb 15, 2021

mbondyra commented Feb 6, 2021 •

edited

Loading

mbondyra Feb 9, 2021 •

edited

Loading

flash1293 left a comment •

edited

Loading

mbondyra Feb 11, 2021 •

edited

Loading