-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Ingest Pipelines Editor] First round of UX improvements #69381
[Ingest Pipelines Editor] First round of UX improvements #69381
Conversation
- Fixed potential text overflow issue on descriptions - Removed border around text input when editing description
@jloleysens can you provide an example test document so I can try the debug flow. And perhaps also an example of a custom processor? |
I usually have just added something like:
|
@mdefazio It's very basic, but you can test using the example in the docs. Processor:
Documents:
|
Here is a test document I use when testing it right now, have to be reformatted a bit from running it in the dev console though, its all testdata generated by me:
|
Yeah I agree that we can improve this! I am happy to iterate a bit on your suggestions.
IIRC one of the designs had the description next to the processor name which visually communicated well that the description was of the processor, but after adding some content into the descriptions the UI started to look a bit messy to me in certain cases when a number of descriptions of different lengths were being added. Imagine the move and edit button are at the very end of the max potential length of the processor description (like first line in the PR description screenshot). There may be a different way to solve this, happy to hear alternate suggestions.
This is a cool idea! Changing the color more drastically of the processors for a stronger visual distinction is a really great idea. Also would like to add this when you enter edit mode, it is currently way too subtle. That being said, I'm not too sold on removing borders on processors. I explored a similar idea with the grey borders and I totally agree that it quickly looks a bit too messy. Are you concerned about the discoverability of the drop zones? |
I'm not sure if my screenshot made enough sense, but my thought was only removing the borders/styling when in 'reorder' mode. But yes, my goal is to try and get the drop zones the focus instead of the rows themselves. (again, only when you click the reorder icon) I'll do some more thinking around the placement of the icons. I see your point and I also know we tried them being on the right side of the row in the index mapping editor and it came with its own problems. |
@elasticmachine merge upstream |
@mdefazio I have updated some of the UX according to your suggestions. I think the dimmed effect makes it much clearer which processor you are currently performing an action on! Let me know what you think and if you have any other feedback about the positioning of buttons. |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
merge conflict between base and head |
…nes/editor-dropzone-refinement * 'master' of github.com:elastic/kibana: (51 commits) Bump jest related packages (elastic#58095) [SECURITY] Introduce kibana nav (elastic#68862) disable pageLoadMetrics job, it's gotten really flaky [Endpoint] Fix flaky endpoints list unit test (elastic#69591) skip failing suite (elastic#69595) [Security_Solution][Endpoint] Resolver leverage ancestry array for queries (elastic#69264) Fixing resolver alert generation (elastic#69587) [Endpoint] add policy empty state (elastic#69449) [APM] Add support for dark mode (elastic#69362) [ML] Data Grid Histograms (elastic#68359) Resolving conflicts (elastic#69597) [DOCS] Add related link to the ingest management docs (elastic#69467) Remove endpoint alert fields from signal mapping (elastic#68934) [ftr] add support for docker servers (elastic#68173) Merge/restyle nodes table (elastic#69098) skip tests using hostDetailsPolicyResponseActionBadge [DOCS] Adds kibana-pull attribute for release docs (elastic#69554) [SIEM][Detection Engine] Fixes 7.8 and 7.9 upgrade issue within rules where you can get the error "params invalid: [lists]: definition for this key is missing" Document authentication settings. (elastic#69284) [CCR] Fix follower indices table not updating after pausing (elastic#69228) ... # Conflicts: # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/pipeline_processors_editor_item/pipeline_processors_editor_item.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/processor_settings_form/processors/custom.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/processors_tree/components/drop_zone_button.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/processors_tree/components/private_tree.tsx
@elasticmachine merge upstream |
Nice work collecting all the various directions/feedback on this.
|
Might be a silly question, but any reason we cannot do the order and moving with drag-n-drop instead? There is plenty of libraries out there doing this already, though I don't know how often we want to include anything third party. Just treating the whole processor box as something that can be pressed and hold to move around, moving it between 2 should just instantly move the other boxes aside, similar to how we do it in other places like dashboards. |
Hey @P1llus ! That's an excellent question, and one we had asked ourselves when starting this project :). The first iteration started out with drag and drop but there were a few reasons we pivoted away from it (you can check out this commit 74b69c7, although I cannot speak to its overall stability 😅). The reasons are:
These points are not to say that we are not open to considering DnD in the future for the tree, I just think for the first iteration it did not quite fit into what we were trying to achieve. There is some thinking going into how we could build that kind of experience on top of what we have, but I hope this response answers your question. |
Thank you very much for the detailed response, it makes it really clear now why it went the way it did and makes perfect sense. Especially around the accessibility and performance on large pipelines, wouldn't really be easy with a keyboard unless you was able to tab through the processors and use the up and down key to move it one up or down, and even then we would still have performance issues. Looking forward to test out the later iterations of the UX! :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🍻 🍻 🍻
This is looking and feeling solid to me! I think there are refinements we can make and clear avenues of exploration we can pursue in separate PRs, but I think this fundamental UX is sound and merge-worthy. Aside from my comments I also had some other thoughts I want to share.
Making "move mode" more obvious
@mdefazio made some good points that the mechanism for exiting move mode (clicking outside of the tree) was non-obvious and that the transition to move mode could also be communicated more obviously.
I suggest adding a "Cancel move" button to the processor that's being moved, which will somewhat address both issues. I'm hoping that by showing this text to the user, it will be clearer that they're in move mode. My hope is that the button provides a clear point of exit.
Clicking outside of the tree to exit move mode would become a nice enhancement rather than the primary mechanism. The current behavior hitting ESC is also a nice enhancement as well as a boon to accessibility. Nice touch there!
In the future we could explore adding a bottom bar when you're in move mode, implementing my EuiMouseTip
suggestion, and/or other ideas.
Processors in move mode
I think this is a big step forward in terms of removing extraneous information when you're in move mode. I suggest two more tweaks to the way a processor is rendered when in this mode:
- I think we should reduce the gap between the processor type and description (above screenshot demonstrates a reduced spacing that I think would work).
- I think we should also hide the "No description" text if there's no description.
This placeholder text is useful to enable the user to do inline editing and add a description. But in move mode the user can't do this, so I think the placeholder text becomes noise.
Processor editor
I suggest changing the label to "Configuration". "Configuration options" seems verbose and it doesn't seem like "options" imparts any additional information.
When there's an error can we disable the "Submit" button? Otherwise the user can click it repeatedly without seeing any response. You have to infer that it's not working because there's an error, but the immediate instinct might be to think something is broken in the UI.
Can we also change the label from "Submit" to "Update"? I think this is a bit more specific to the action being performed. We can also add an "Update processor" tooltip to the edit icon in the processor tree item, for some continuity.
@@ -52,7 +52,10 @@ export const registerGrammarChecker = (editor: monaco.editor.IEditor) => { | |||
|
|||
const updateAnnos = async () => { | |||
const { annotations } = await wps.getAnnos(); | |||
const model = editor.getModel() as monaco.editor.ITextModel; | |||
const model = editor.getModel() as monaco.editor.ITextModel | null; | |||
if (!model) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, what are the scenarios in which there wouldn't be a model?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is quite specific to how monaco works, and tbh I am not entirely certain 😅 . They do provide types that support this claim and I have seen that, on occasion, at runtime when editing the model is unavailable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for explaining! Might be worth leaving a comment in the code with this info.
.../pipeline_processors_editor/components/pipeline_processors_editor_item/inline_text_input.tsx
Outdated
Show resolved
Hide resolved
...essors_editor/components/pipeline_processors_editor_item/pipeline_processors_editor_item.tsx
Outdated
Show resolved
Hide resolved
...essors_editor/components/pipeline_processors_editor_item/pipeline_processors_editor_item.tsx
Outdated
Show resolved
Hide resolved
} | ||
); | ||
|
||
const cannotMoveHereLabel = i18n.translate( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder whether things would feel simpler to the user to just be allowed to move the processor anywhere, even if it's just to "drop" it back in place. If you click move accidentally and want to get out of that mode, would this be convenient?
Put another way: are we helping the user by telling them they can't move the processor above or below where it currently is? Technically, they could move the processor there -- it just wouldn't change position. If a user did this, would that experience be so disorienting that we'd want to prevent it?
I'm just asking these questions aloud, I don't have any answers. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, that is an interesting point! And kind of philosophical 😄 . If something is moved, but its position did not change did it move?
I gave this some thought; the way I think about it is "wouldn't change position" is equivalent to not moving and this to me is the noop described by cannot move.
The primary goal I think this serves at the moment is giving us a way to make drop zones more discoverable (and the entire drop mode).
...cation/components/pipeline_processors_editor/components/processors_tree/processors_tree.scss
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per our conversation, approving to move forward with gathering feedback.
@elasticmachine merge upstream |
- also hide the description in move mode when it is empty - update and refactor some shared sass variables - some number of sass changes to make labels play nice in move mode - changed the logic to not render the buttons when in move mode instead of display: none on them. The issue is with the tooltip not hiding when when we change to move mode and the mouse event "leave" does get through the tooltip element causing tooltips to hang even though the mouse has left them.
- Monaco XJSON worker was not handling trailing whitespace - Update copy in the processor configuration form
💚 Build SucceededBuild metrics
History
To update your PR or re-run it, just comment with: |
* First round of UX tweaks - Fixed potential text overflow issue on descriptions - Removed border around text input when editing description * Updated the on-failure pipeline description copy * Properly encode URI component pipeline names * use xjson editor in flyout * also hide the test flyout if we are editing a component * add much stronger dimming effect when in edit mode * also added dimming effect to moving state * remove box shadow if dimmed * add tooltips to dropzones * fix CITs after master merge * fix nested rendering of processors tree * only show the tooltip when the dropzone is unavaiable and visible * keep white background on dim * hide controls when moving * fix on blur bug * Rename variables and prefix booleans with "is" * Remove box shadow on all nested tree items * use classNames as it is intended to be used * Refactor SCSS values to variables * Added cancel move button - also hide the description in move mode when it is empty - update and refactor some shared sass variables - some number of sass changes to make labels play nice in move mode - changed the logic to not render the buttons when in move mode instead of display: none on them. The issue is with the tooltip not hiding when when we change to move mode and the mouse event "leave" does get through the tooltip element causing tooltips to hang even though the mouse has left them. * Fixes for monaco XJSON grammar parser and update form copy - Monaco XJSON worker was not handling trailing whitespace - Update copy in the processor configuration form Co-authored-by: Elastic Machine <[email protected]>
…0076) * First round of UX tweaks - Fixed potential text overflow issue on descriptions - Removed border around text input when editing description * Updated the on-failure pipeline description copy * Properly encode URI component pipeline names * use xjson editor in flyout * also hide the test flyout if we are editing a component * add much stronger dimming effect when in edit mode * also added dimming effect to moving state * remove box shadow if dimmed * add tooltips to dropzones * fix CITs after master merge * fix nested rendering of processors tree * only show the tooltip when the dropzone is unavaiable and visible * keep white background on dim * hide controls when moving * fix on blur bug * Rename variables and prefix booleans with "is" * Remove box shadow on all nested tree items * use classNames as it is intended to be used * Refactor SCSS values to variables * Added cancel move button - also hide the description in move mode when it is empty - update and refactor some shared sass variables - some number of sass changes to make labels play nice in move mode - changed the logic to not render the buttons when in move mode instead of display: none on them. The issue is with the tooltip not hiding when when we change to move mode and the mouse event "leave" does get through the tooltip element causing tooltips to hang even though the mouse has left them. * Fixes for monaco XJSON grammar parser and update form copy - Monaco XJSON worker was not handling trailing whitespace - Update copy in the processor configuration form Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Elastic Machine <[email protected]>
@jloleysens After watching the recording of @cjcenizal feedback on the design, I had the following comments in case they can be useful:
I also had a question: I am not sure I follow why we don't want ES engineer to give us a unique "name" field instead of the "description". Name can be multiple words but would be a text input. Description means multiple paragraphs and would require a textarea. I wonder how is the search experience going to look like with paragraphs to search into instead of keywords? |
@sebelga Thanks for this feedback, I really like your idea regarding putting the move button to the left and using badges for the names. I think in the context of a processor the expectation is not necessarily that users write that much text, because generally I don't think there will be much to describe 😅. The intention of the description field is more in line with what you have described in giving users a way to uniquely name things. At the time we have chosen the term description. |
Summary
Pretty faithful implementation of this #66021 (comment) by @cjcenizal to improve the click tree experience.
Also included some minor style tweaks for the descriptions field and reordered the buttons to be before the description.
How to test
At this time, to test adding and persisting descriptions you would need to build ES master from source. See the details of the
yarn es source
command. TL;DR fork ES repo in a sibling directory to Kibana and runyarn es source
inside of kibana to build from source.Screenshots
Description with text overflow
New move UX
Gifs