Skip to content
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

[EuiDragDrop] Inefficiencies for specific use-case #4024

Closed
cchaos opened this issue Sep 9, 2020 · 11 comments
Closed

[EuiDragDrop] Inefficiencies for specific use-case #4024

cchaos opened this issue Sep 9, 2020 · 11 comments

Comments

@cchaos
Copy link
Contributor

cchaos commented Sep 9, 2020

When we originally implemented this piece of UI (elastic/kibana@74b69c7) we were using the DnD component provided by EUI. For the specific task of building ingest pipelines we hit a few snags:

  1. Processors in pipelines nest into one another. This means drop sites need to nest into one another -- something react-beautiful-dnd (the lib used by EUI) does not support (Support nested list (drag item from parent list into nested child list) atlassian/react-beautiful-dnd#1001)
  2. Processor pipelines can get tall. Really tall. In some cases the UI will have to render ~150 processors. Dragging is not a very suitable UX in this case for certain actions where users might want to copy a processor and move it some 100 processors down (or up, or into another processor).
  3. With the way that react-dnd-works we started seeing performance issues when you start dragging (a slight lag in responsiveness) - this was sort of fixable by using react-virtualize (or react-window which this still needs to migrate to), but I'm not sure it was to an acceptable level.

For those reasons we pivoted to something better suited to the case of nested drop sites and really tall lists that need to be as performant as possible. Of the reasons I mentioned I'd say the first two are the most important. It would be cool to chat further about this specific problem and perhaps it is something EUI can come to solve?

Originally posted by @jloleysens in elastic/kibana#76885 (comment)

@seeruk
Copy link
Contributor

seeruk commented Sep 22, 2020

What was the solution you pivoted to in the end? We're using the drag and drop component on page with a lot of nested and sibling drag / drop targets and we've noticed some serious performance issues. We're basically building a large dynamic form, and we've resolved all other performance issues bar the ones caused by the drag and drop components.

From looking into it a bit, the lack of useCallback seems to be one issue, the other is that all EuiDroppable and EuiDraggable components under the context re-render if the context component renders. I'm struggling to see how that can be avoided though at the moment, but it causes a pretty huge amount of lag. I'm still looking into some approaches to fix this, but am keen to understand what solution you've chosen!

Edit: Just taken a look at that other issue, I can see the gif of the other functionality, but I'm not totally sure what's happening there. Is it a click and drop sort of thing? (I quite like this idea for mobile use-cases too actually) Where you toggle a mode where you can move things, and it shows you where you can move something to and then you click where it should go?

@jloleysens
Copy link

Hey @seeruk ! We ended up creating a custom drag and drop editor specifically for wrangling tall processor pipelines, permalink to where component is being exported: (https://github.com/elastic/kibana/blob/4b6d77fa5d5e3a830ccaabad863252044f7c1523/x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/index.ts#L1).

I also saw the issue you are describing regarding dnd context causing all children to re-render with no way to memoize the child contents which creates a jump the moment you start dragging. I noticed this for even moderately sized lists. This seems to be something that should be addressed in react-beautiful-dnd because EUI is really a wrapper around that library.

Is it a click and drop sort of thing? (I quite like this idea for mobile use-cases too actually)

Yes, it is a click and drop instead of click-and-hold UX.

@jloleysens
Copy link

The other thing, for performance, that is worth checking out is https://github.com/atlassian/react-beautiful-dnd/blob/master/docs/patterns/virtual-lists.md. I'd be curios to hear whether this addresses the performance problem.

@seeruk
Copy link
Contributor

seeruk commented Sep 23, 2020

Just given that virtual mode on the droppables a try, it seems to still render them even if they're off-screen. I'm not totally sure how it's detecting when to render or not. It might not be React DnD though.

From the experiments I did last night I think there's something EUI is doing on top of React DnD with the EuiDragDropContext, it seems to make all of the draggable and droppable components re-render any time the context is re-rendered. I did manage to stop euiOnDragStart and euiOnDragEnd from changing unnecessarily by using useCallback, but I think when the child component changes, causing the context component to change, it ends up recreating the context value and passing it to all of the consumers of that hook, causing all of them to re-render. This part at least seems to be in the EUI layer. I'm going to look into it a bit more, and if I make a breakthrough then I'll let you know, hopefully with a PR.

Edit: I think this first caveat is actually what's being run into as the final issue: https://reactjs.org/docs/context.html#caveats - just testing out that theory now, changing the context value to solely be an EuiDraggingType, and using useCallback on the drag start / end functions defined in the component.

Edit 2: No luck with that :(

@cjcenizal
Copy link
Contributor

cjcenizal commented Sep 25, 2020

For reference, here's what the click and drop interaction looks like, though the design of the items that are being moved has changed since I captured this GIF:

moving_processor_ux

@github-actions
Copy link

👋 Hey there. This issue hasn't had any activity for 180 days. We'll automatically close it if that trend continues for another week. If you feel this issue is still valid and needs attention please let us know with a comment.

@cjcenizal
Copy link
Contributor

Is drag and drop used elsewhere in lists in Kibana? I wonder what the UX is like there. I wonder if people generally find it fatiguing to drag and drop in long lists.

@myasonik
Copy link
Contributor

There's a lot of DnD in Lens (in and outside lists) and they've also shipped what I see to be some of the best accessibility features baked in. I'd love it if it was possible to pull the Lens solution out of Kibana, abstract it for EUI, and then share it across everywhere we have DnD. I'm sure it's not so easy though...

To your point @cjcenizal, Lens' keyboard controls are a first-class citizen and I often use them instead of dragging.

That said, I'm not sure if Lens has tackled any performance issues or nesting as mentioned at the top of this issue.

@mbondyra maybe you have some more thoughts on this?

@mbondyra
Copy link
Contributor

mbondyra commented Mar 31, 2021

In our case we don't really have very long lists in drag and drop with both draggable and droppable elements so we don't optimize for this case.

For example the datasource field list panel can be only draggable so we don't rerender all the elements when starting dragging, just the dragged one. As for configuration panel, we don't expect having more than 5-10 draggable/droppable elements maximum as the chart would become unreadable for the user for larger amount. I tried testing adding many operations just to check how it behaves - there's a slowdown for animations, but that's expected as we didn't optimize for long lists, but I think it could be improved if needed.

Mar-31-2021 08-47-13

I think abstracting Lens drag and drop wouldn't be extremely complex, but it would be probably quite time consuming. Plus we would need to clearly define what are requirements - Lens usecase is still quite specific and even looking at the example above, I can tell we haven't implemented this usecase (child-parent nesting, where child can become a parent and vice-versa)

@github-actions
Copy link

👋 Hey there. This issue hasn't had any activity for 180 days. We'll automatically close it if that trend continues for another week. If you feel this issue is still valid and needs attention please let us know with a comment.

@github-actions
Copy link

github-actions bot commented Oct 4, 2021

❌ We're automatically closing this issue due to lack of activity. Please comment if you feel this was done in error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants