Skip to content

Conversation

@jloleysens
Copy link
Contributor

@jloleysens jloleysens commented Jul 17, 2020

Summary

Adds a tooltip to ingest processors editor UI to indicate which processor is being moved. See images below.

How to test

  1. Start Kibana with default/basic license
  2. Create the following pipeline https://gist.github.com/jloleysens/e9c92d0f26e16af2ea301fa7b6349815
  3. Go to the ingest pipelines plugin in the management section. The first processor should have a description
  4. Click the move button on the first processor
  5. Scroll to the bottom of the list and place the processor last

Throughout these steps, when the tooltip is visible, it should stay roughly 20px below and 20px to the right of the cursor (even while scrolling).

GIF

tooltip

Screenshots

Move processor with description
Screenshot 2020-07-17 at 10 10 22

Move processor without description
Screenshot 2020-07-17 at 10 10 33

Checklist

@jloleysens jloleysens added v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more t// release_note:skip Skip the PR/issue when compiling release notes v7.10.0 Feature:Ingest Node Pipelines Ingest node pipelines management labels Jul 17, 2020
@jloleysens jloleysens requested a review from cjcenizal July 17, 2020 08:33
@jloleysens jloleysens requested a review from a team as a code owner July 17, 2020 08:33
@elasticmachine
Copy link
Contributor

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

Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

Nice work JL! I think this is a sweet UX enhancement. I have a few suggestions and found a couple bugs. If you'd like to address these separately I can create issues for them. Just let me know if you'd like me to do that.

Component suggestion: EuiMouseTip

I'm pretty excited by the prospect of an EuiMouseTip component! I know we already discussed this, I just wanted to get my thoughts in writing. It could have the same position prop (and positioning logic!) as EuiToolTip, so consumers can specify a position or let it dynamically reposition as the mouse nears the edges of the browser window. Then you could get rid of PipelineProcessorsItemTooltip and your ProcessorInformation implementation could be:

<EuiMouseTip position="bottom">
  {/* snip */}
</EuiMouseTip>

Component suggestion: EuiInlineEditable

Let's see if this is a pattern that EUI can adopt and evangelize. A component that makes inline editing easy would be a great way to encourage widespread adoption. Something kinda like this:

{/* This component basically just handles a focus event to show/hide `children` and manages focus-setting (possibly with some ref-passing?) */}
<EuiInlineEditable value={fieldValue}>
  <EuiFieldText value={fieldValue} onChange={/* ... */} />
</EuiInlineEditable>

UX suggestion: Moving between regular and failure processors

I just noticed that if there aren't any failure processors you can't move a processor into the list.

image

UX suggestion: "Can't move here" feedback

There's a lot of visual feedback in the screenshot below: a grayed-out drop indicator, a "Cannot move here" tooltip, a "Blocked" cursor, and the new processor tooltip. It's a lot to take in! Too much, I think.

I think the key to solving this "information overload" is to recognize the contradiction in showing a drop indicator (which is our way of telling the user "You can move here"), and then showing a "Cannot move here" tooltip to countermand the drop indicator. (I know the drop indicator grays out to show it's a no-op but I think this is too subtle so I'm ignoring it in this assessment). If we remove both, then I think we satisfy our goal of telling the user that they can't move the processor there. The rules are simpler to learn: if there's a drop indicator then you can move there and if there isn't then you can't.

I also suggest we remove the "Not allowed" cursor. I'm just not sure it's common enough for people to intuit it's meaning here. It seems I'm not the only one: https://ux.stackexchange.com/questions/12726/correct-cursor-for-content-cannot-be-interacted-with

image

Bug: Moving to top or bottom of processors list

There are two ReactVirtualized nested divs which contain the processors list. These divs have overflow: hidden set on them, which I believe is required for virtualized scrolling. However, this clips the drop zone buttons at the top and bottom of the list, which slightly degrades the UX when moving a processor to the top or bottom because the user has to acquire a smaller target and so is more likely to overshoot. It's still possible to move processors to the top/bottom, but it's just not as easy as it could be.

Setting overflow: visible on the ReactVirtualized divs solves the problem but probably causes problems with scrolling. I didn't test this though.

Bug: Moving while processor panel is open

When the processor panel is open, actions on each processor appear disabled but it's still possible to click the "Move" button, which closes the processor panel. It's not obvious if doing so will cancel the save the user's input in the processor panel, which is a poor UX. I suggest disabling the move button when the processor panel is open, the same as the other actions.

image

@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

@jloleysens
Copy link
Contributor Author

Component suggestion: EuiMouseTip

👆🏻 this is something I plan on discussing with the EUI team for sure, I'll be opening an issue on their repo soon! Thanks for weighing in with your thoughts, I'll be sure to factor them in to our discussion (and ping you as needed!)

Component suggestion: EuiInlineEditable

This is something I had not thought about as much but I think certainly deserves some mention EUI-side too. This certainly merits a separate discussion/issue on EUI.

Bug: Moving to top or bottom of processors list

I'm not sure I was able to replicate this to the extent that I'd view it as a bug. That being said, with the work identified in the a11y issue (#71666) we will be swapping out react-virtualized for react-window. This might influence the behaviour you have identified - we can review it then again.

Bug: Moving while processor panel is open

Great catch! This was a particularly weird one because I was passing in disabled to EuiButtonToggle which was correctly updating the style to look disabled. But at a functional level the button was still clickable. I fixed this by using the correct EUI API isDisabled and added a test for this too.

@jloleysens
Copy link
Contributor Author

UX suggestion: Moving between regular and failure processors

We can definitely create an issue for tracking this! It is a pre-existing issue. Would you mind opening one?

UX suggestion: "Can't move here" feedback

Thanks for sharing your insight here! I can see how the not-allowed cursor is perhaps adding unnecessary noise - removed it :)

As for the grey line and "cannot move here" tooltip; I think your point about noise is valid but only for users that are familiar with the functionality. In those cases I don't even think those users will see the "cannot move here" UI appearing because they will move over it too fast (from what I have observed). That leaves users who are new to this UI - for them I think the extra clear indication about entering "move" mode and introducing the drop zones off the bat is still valuable for discoverability.

For those reasons I'd prefer to punt on this point a little bit longer.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
ingestPipelines 267 +9 258

async chunks size

id value diff baseline
ingestPipelines 584.5KB +6.9KB 577.6KB

History

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

@jloleysens jloleysens merged commit e5c7e9a into elastic:master Jul 20, 2020
@jloleysens jloleysens deleted the ingest-pipelines/tooltip branch July 20, 2020 15:03
jloleysens added a commit to jloleysens/kibana that referenced this pull request Jul 20, 2020
* first implementation of tooltip

* Add processor tooltip component files

* remove init position from code for now

* colocate on change handler and make code a bit cleaner

* removed document.body.appendChild logic because EuiPortal does that for us

* use correct toggle button api

* added test to check button disabled while editing

* remove cursor not allowed

* simplify logic

* assert if against positive

* remove unused variable

* Remove unused actions const

Co-authored-by: Elastic Machine <[email protected]>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jul 20, 2020
* master: (60 commits)
  [SIEM][Detection Engine][Lists] Adds list permissions (elastic#72335)
  [SIEM][Detection Engine][Lists] Adds conflict versioning and io-ts improvements to lists (elastic#72337)
  [Resolver] no longer pass related event stats to process node component (elastic#72435)
  Revert "skip flaky suite (elastic#72146)"
  [Security Solution] Cleanup endpoint telemetry (elastic#71950)
  Unskip dashboard embeddable rendering tests (elastic#71824)
  [ENDPOINT] Added unerolling status for host. (elastic#72303)
  [Alerting][Connectors] Increase the size of the logos (elastic#72419)
  [SECURITY] [Timeline] Raw events not displayed (elastic#72387)
  [ML] Fixes display of regression stop stats if one is NaN (elastic#72412)
  [Ingest Pipelines] Processor Editor Move Tooltip (elastic#72239)
  Fix match phrase and not match phrase comparators (elastic#71850)
  [Plugin Generator] Generate tsconfig and useDefaultBehaviors (elastic#72040)
  [Security Solution][Timeline] Fix timeline styling and createFrom beh… (elastic#72152)
  [Resolver] Selector performance (elastic#72380)
  [Ingest Manager] Set `_meta` in the index.mappings (elastic#72026)
  [Ingest Manager] Do not bumb config revision during config creation (elastic#72270)
  [ML] Adding missing index pattern name to new job wizards (elastic#72400)
  [ML] improve annotation flyout performance (elastic#72299)
  [APM] Testing error rate API and restructuring folders (elastic#72257)
  ...
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jul 20, 2020
* master: (26 commits)
  [SIEM][Detection Engine][Lists] Adds list permissions (elastic#72335)
  [SIEM][Detection Engine][Lists] Adds conflict versioning and io-ts improvements to lists (elastic#72337)
  [Resolver] no longer pass related event stats to process node component (elastic#72435)
  Revert "skip flaky suite (elastic#72146)"
  [Security Solution] Cleanup endpoint telemetry (elastic#71950)
  Unskip dashboard embeddable rendering tests (elastic#71824)
  [ENDPOINT] Added unerolling status for host. (elastic#72303)
  [Alerting][Connectors] Increase the size of the logos (elastic#72419)
  [SECURITY] [Timeline] Raw events not displayed (elastic#72387)
  [ML] Fixes display of regression stop stats if one is NaN (elastic#72412)
  [Ingest Pipelines] Processor Editor Move Tooltip (elastic#72239)
  Fix match phrase and not match phrase comparators (elastic#71850)
  [Plugin Generator] Generate tsconfig and useDefaultBehaviors (elastic#72040)
  [Security Solution][Timeline] Fix timeline styling and createFrom beh… (elastic#72152)
  [Resolver] Selector performance (elastic#72380)
  [Ingest Manager] Set `_meta` in the index.mappings (elastic#72026)
  [Ingest Manager] Do not bumb config revision during config creation (elastic#72270)
  [ML] Adding missing index pattern name to new job wizards (elastic#72400)
  [ML] improve annotation flyout performance (elastic#72299)
  [APM] Testing error rate API and restructuring folders (elastic#72257)
  ...
@cjcenizal
Copy link
Contributor

@jloleysens I created #72568.

jloleysens added a commit that referenced this pull request Jul 21, 2020
* first implementation of tooltip

* Add processor tooltip component files

* remove init position from code for now

* colocate on change handler and make code a bit cleaner

* removed document.body.appendChild logic because EuiPortal does that for us

* use correct toggle button api

* added test to check button disabled while editing

* remove cursor not allowed

* simplify logic

* assert if against positive

* remove unused variable

* Remove unused actions const

Co-authored-by: Elastic Machine <[email protected]>

Co-authored-by: Elastic Machine <[email protected]>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jul 21, 2020
…feature-privileges

* alerting/consumer-based-rbac: (45 commits)
  fixed alerts test
  [SIEM][Detection Engine][Lists] Adds list permissions (elastic#72335)
  [SIEM][Detection Engine][Lists] Adds conflict versioning and io-ts improvements to lists (elastic#72337)
  [Resolver] no longer pass related event stats to process node component (elastic#72435)
  Revert "skip flaky suite (elastic#72146)"
  [Security Solution] Cleanup endpoint telemetry (elastic#71950)
  Unskip dashboard embeddable rendering tests (elastic#71824)
  [ENDPOINT] Added unerolling status for host. (elastic#72303)
  [Alerting][Connectors] Increase the size of the logos (elastic#72419)
  [SECURITY] [Timeline] Raw events not displayed (elastic#72387)
  [ML] Fixes display of regression stop stats if one is NaN (elastic#72412)
  [Ingest Pipelines] Processor Editor Move Tooltip (elastic#72239)
  Fix match phrase and not match phrase comparators (elastic#71850)
  [Plugin Generator] Generate tsconfig and useDefaultBehaviors (elastic#72040)
  [Security Solution][Timeline] Fix timeline styling and createFrom beh… (elastic#72152)
  allow user to disable alert even if they dont have privileges to the underlying action
  [Resolver] Selector performance (elastic#72380)
  [Ingest Manager] Set `_meta` in the index.mappings (elastic#72026)
  [Ingest Manager] Do not bumb config revision during config creation (elastic#72270)
  [ML] Adding missing index pattern name to new job wizards (elastic#72400)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature:Ingest Node Pipelines Ingest node pipelines management release_note:skip Skip the PR/issue when compiling release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more t// v7.10.0 v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants