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.

Sign up for GitHub

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

Jump to bottom

[Ingest Pipelines] Drop into an empty tree #76885

Merged
merged 4 commits into from
Sep 14, 2020
Merged

[Ingest Pipelines] Drop into an empty tree #76885

merged 4 commits into from
Sep 14, 2020

Conversation

jloleysens
Copy link
Contributor

@jloleysens jloleysens commented Sep 7, 2020 •
edited
Loading

Summary

Add the ability to drop a processor into an empty tree

Gif

move-processor-to-empty-tree

Checklist

Delete any items that are not applicable to this PR.

Release note

The ingest node pipeline editor now has the ability to move processors into an empty tree.

@jloleysens jloleysens added enhancement New value added to drive a business result v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more release_note:skip Skip the PR/issue when compiling release notes v7.10.0 Feature:Ingest Node Pipelines Ingest node pipelines management labels Sep 7, 2020
@jloleysens jloleysens requested review from a team as code owners September 7, 2020 15:12
@elasticmachine
Copy link
Contributor

Pinging @elastic/es-ui (Team:Elasticsearch UI)

@cjcenizal
Copy link
Contributor

FWIW this seems release note worthy, using the content already provided in the PR description. Why not add the release_note:enhancement label?

@jloleysens jloleysens added release_note:enhancement and removed release_note:skip Skip the PR/issue when compiling release notes labels Sep 8, 2020
@jloleysens
Copy link
Contributor Author

@cjcenizal I added a release note and updated the labels. Would you mind reviewing :D?

miukimiu
miukimiu approved these changes Sep 8, 2020
View reviewed changes
Copy link
Contributor

@miukimiu miukimiu left a comment

Choose a reason for hiding this comment

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

I just reviewed the CSS that was added and LGTM!

@cchaos
Copy link
Contributor

cchaos commented Sep 8, 2020

Hey @jloleysens , just wanted to throw a suggestion to you, but doesn't need to be tackled in this PR. It looks like you've implemented a drag-and-drop interface with these panels. EUI has some components to help create this behavior and implements some consistent look to droppable areas (the green highlight). It also moves other items around nicely so that it is easy to see where the placement is happening as well as some keyboard commands for accessibility.

Screen Recording 2020-09-08 at 09 51 24 AM

It would be great if this page could use the DnD components provided by EUI mainly to help create a consistent behavior for DnD everywhere in Kibana so user's can maintain that knowledge of what parts can be dragged and to where.

@jloleysens
Copy link
Contributor Author

jloleysens commented Sep 9, 2020 •
edited
Loading

Hi @cchaos ! Thanks for commenting here :)

When we originally implemented this piece of UI (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?

@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

@cchaos
Copy link
Contributor

cchaos commented Sep 9, 2020

Thanks for the context @jloleysens ! This is definitely really great feedback to bring back to the @elastic/eui-design team. We don't tend to find opportunities to stress test our components so we don't catch instances like this. It would be great if we could find a resolution on the EUI side to help you out. My main concern here is using different visuals to represent drag and drop than the rest of Kibana. So if, in the end, EUI's DnD solution still doesn't quite work, lets figure out a way to align the visuals.

@cchaos cchaos mentioned this pull request Sep 9, 2020
Closed
cjcenizal
cjcenizal approved these changes Sep 10, 2020
View reviewed changes
Copy link
Contributor

@cjcenizal cjcenizal left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Nice GIF! Tested locally, code looks good. I think this is shippable, but it would be nice to fix the keyboard accessibility in a later PR.

processor_move_accessibility

It looks like the selected processor shifts out of position as soon as I tab forward, and the dropzone also doesn't highlight once I tab to it. I would also expect to be able to hit enter to drop it, but that doesn't work.

I didn't test this, but when the dropzone is highlighted I would also expect the screenreader to identify the action I can perform at the location I'm at, for example "Press enter to move the processor to the failure processors".

@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

async chunks size

id value diff baseline
ingestPipelines 739.4KB +982.0B 738.5KB

History

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

@jloleysens
Copy link
Contributor Author

Thanks for the review @cjcenizal !

Ref #71666

@jloleysens jloleysens merged commit 97813b2 into elastic:master Sep 14, 2020
@jloleysens jloleysens deleted the fix/ingest-pipelines/drop-into-empty-tree branch September 14, 2020 10:46
jloleysens added a commit to jloleysens/kibana that referenced this pull request Sep 14, 2020
@jloleysens @elasticmachine
[Ingest Pipelines] Drop into an empty tree (elastic#76885)
111bfb9 
* add ability to drop processor into empty tree

* added a test

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@jloleysens jloleysens mentioned this pull request Sep 14, 2020
Merged
jloleysens added a commit that referenced this pull request Sep 14, 2020
@jloleysens @elasticmachine
[Ingest Pipelines] Drop into an empty tree (#76885) (#77312)
f68c597 
* add ability to drop processor into empty tree

* added a test

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Sep 14, 2020
@gmmorris
Merge branch 'master' into alerting-exempt-rbac
a0fa54e 
* master: (26 commits)
  updating datatable type (elastic#77320)
  [ML] Fix custom URLs processing for security app (elastic#76957)
  [telemetry] add schema guideline + schema_check new check for --path config (elastic#75747)
  [ML] Transforms: API schemas and integration tests (elastic#75164)
  [Mappings editor] Add support for wildcard field type (elastic#76574)
  [Ingest Manager] Fix flyout instruction selection (elastic#77071)
  [Telemetry Tools] update lodash to 4.17 (elastic#77317)
  [APM] Service inventory redesign (elastic#76744)
  Hide management sections based on cluster/index privileges (elastic#67791)
  [Snapshot Restore] Disable steps when form is invalid (elastic#76540)
  [Mappings editor] Add support for positive_score_impact to rank_feature (elastic#76824)
  Update apm.ts (elastic#77310)
  [OBS] Remove beta badge, change news feed size and add external icon to news feed link (elastic#77164)
  [Discover] Convert legacy sort to be compatible with multi sort (elastic#76986)
  [APM] API Snapshot Testing (elastic#77229)
  [ML] Functional tests - increase wait time for DFA start (elastic#77307)
  [UiActions][Drilldowns] Fix actions sorting in context menu (elastic#77162)
  [Drilldowns] Wire up new links to new docs (elastic#77154)
  Fix APM issue template
  [Ingest Pipelines] Drop into an empty tree (elastic#76885)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjcenizal cjcenizal cjcenizal approved these changes

@miukimiu miukimiu miukimiu approved these changes

Assignees
No one assigned
Labels
enhancement New value added to drive a business result Feature:Ingest Node Pipelines Ingest node pipelines management release_note:enhancement Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.10.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@jloleysens @elasticmachine @cjcenizal @cchaos @kibanamachine @miukimiu