[Lens] Refactor drag and drop

[mbondyra]

Summary

When I created drag and drop for Lens, the API I went for was not the most readable one. It was designed this way because I wanted to gain some performance, but it was very hard to maintain the performance gain with a lot of changes in the drag and drop area because all the pieces of the code needed to memoized in a tricky way and it wasn't communicated well.
In the end it works even without these tricks so I decided to simplify it in this PR.

The main changes include:

1. Instead of multiple useState per parameter, we keep all the state in reducer both for ReorderProvider and RootDragDropProvider. Thanks to that we get multiple improvements:
2. The code in DragDrop component becomes more descriptive as we don't run multiple state updates when user executes an action but one state update describing what actually happens (eg. dispatchDnd({type: 'selectDropTarget' ....}). The internal logic of the update lives in the reducer.
3. We don't have to pass trackUiCounterEvents as another prop to DragDrop and run it wherever we need - instead we pass it as a middleware to the context and run before dispatching (and it's very easy to add more middlewares if we need extra integrations at some point!)
4. We also run a11y announcements as a middleware instead of inside DragDrop component
5. The ChildDragDropProvider props look much cleaner:
   before:

      <ChildDragDropProvider
        keyboardMode={keyboardModeState}
        setKeyboardMode={setKeyboardModeState}
        dragging={draggingState.dragging}
        setA11yMessage={setA11yMessage}
        setDragging={setDragging}
        activeDropTarget={activeDropTargetState}
        setActiveDropTarget={setActiveDropTarget}
        registerDropTarget={registerDropTarget}
        dropTargetsByOrder={dropTargetsByOrderState}
        dataTestSubjPrefix={dataTestSubj}
        onTrackUICounterEvent={onTrackUICounterEvent}
      >
        {children}
      </ChildDragDropProvider>

after:

<ChildDragDropProvider value={[state, dispatch]}>{children}</ChildDragDropProvider>

6. Created custom hook useDragDropContext instead of using useContext(DragContext) and making DragContext private. This way we will avoid potential problems with using context outside of root.
7. Bonus thing - if we ever decide to move to redux, the structure is there already

What I am still not happy with is that the tests are very domain-dependant instead of user-driven - instead of checking the store actions, I should check the interface from the user perspective. I will try to work on it once I find some time between more important tasks though.

[elasticmachine]

Pinging @elastic/kibana-visualizations @elastic/kibana-visualizations-external (Team:Visualizations)

[mbondyra]

this param is actually not needed - it was added when the implementation was different. Basically it checks if we want to add the information to the user about the extra keys (eg 'shift to swap') and if the key is pressed (so swap is highlighted) then didn't inform the user about the keys. I think it's better to always inform the user about modifier keys so I removed it.

[stratoula]

This looks good to me, I am not an dnd expert but I like the simplification of the api. @jughosta can you also take a look here please?

I wonder why a simple change has increased the async bundle of event annotations 🤔

[mbondyra]

@stratoula I managed to fix the bundle issue. The problem was caused by moving a11y announcements to the RootDragDropProvider, which made the bundle size bigger. After digging deeper, I realized that we were unnecessarily importing the provider into the annotations plugin and surrounding annotations with it. I removed this import from annotation plugin. To simplify things I also decided to change directly accessing the context useContext(DragContext) but using a custom hook useDragDropContext that not only fetches the context but also checks if we're currently within its scope. This tweak should do the trick and prevent us from bugs difficult to catch. 😊

[stratoula]

Wow @mbondyra, thank you so much for looking into it. These small nits make our products better!

[stratoula]

changes LGTM in case of green CI if @jughosta is also fine!

[jughosta]

Great work!

[jughosta]

Previously it was necessary for reorder styles to be applied only after drag started. Was this behaviour changed?

[mbondyra]

Yes, it's from the PR before this one - the styles are applied inside the DragDrop component where we check for dragging so no need to do the check here as we only have one type of dropTypes for annotations.

[drewdaemon]

I like it.

[drewdaemon]

As discussed offline, this removal breaks the gated draggable dimension button interface (sorry—easy to miss 😅 ). We should fix this before merging!

[mbondyra]

Just for the context – I thought this is an extra provider that we use inside Lens but as it works in the table list, we still need it. Thank you so much Drew for keeping an eye on it 🙏🏼

[drewdaemon]

It looks like something changed with the ghost:

Before this PR

After this PR (ghost is present before I drag)

Could it be related to this change?

[drewdaemon]

Would be nice to get that ghost problem fixed, but approving so as not to hold you up when I'm OOO!

[mbondyra]

Thank you @drewdaemon, yep, it is the problem with this one indeed. I did fix it in another PR but actually seeing some other issues that it caused I reverted it. While checking I added reordering to annotation group editing here in this PR - it's hidden under the feature var, so if there's any problem it's not visible for the users.

[kibana-ci]

💚 Build Succeeded

• Buildkite Build
• Commit: 17b52fe
• Webpack Bundle Analyzer

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
@kbn/dom-drag-drop	34	26	-8

Async chunks

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

id	before	after	diff
discover	542.4KB	542.0KB	-470.0B
eventAnnotation	120.7KB	137.9KB	+17.2KB
lens	1.3MB	1.3MB	-16.0B
total			+16.7KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
@kbn/dom-drag-drop	4	5	+1

Unknown metric groups

API count

id	before	after	diff
@kbn/dom-drag-drop	52	39	-13

ESLint disabled line counts

id	before	after	diff
@kbn/dom-drag-drop	0	1	+1
enterpriseSearch	14	16	+2
securitySolution	408	412	+4
total			+7

Total ESLint disabled count

id	before	after	diff
@kbn/dom-drag-drop	0	1	+1
enterpriseSearch	15	17	+2
securitySolution	487	491	+4
total			+7

History

• 💔 Build #141517 failed 7eb237e7c08aaa94adc41eb01697fd44d8f90a26
• 💚 Build #141296 succeeded da637a37ee7571cdb492c153c77c851dbed1a6ef
• 💚 Build #141252 succeeded 608b94d18471c19870ae286d1d840d4caa21e197
• 💚 Build #141143 succeeded 4057615feb9ff0b57f22c90e25c78e7e7f17db19
• 💔 Build #141113 failed 4d9886fbaf4c732a8f45b51b83e6d8543e3b181d
• 💔 Build #141111 failed f8c343bc95f96d65b253423fe520763495d53810

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