Skip to content

Always show row actions for the fields list in the Mappings Editor#53731

Merged
cjcenizal merged 14 commits intoelastic:feature/mappings-editorfrom
cjcenizal:mappings-editor/surface-field-actions
Jan 14, 2020
Merged

Always show row actions for the fields list in the Mappings Editor#53731
cjcenizal merged 14 commits intoelastic:feature/mappings-editorfrom
cjcenizal:mappings-editor/surface-field-actions

Conversation

@cjcenizal
Copy link
Contributor

@cjcenizal cjcenizal commented Dec 20, 2019

Closes #53012

In this PR:

  • We always shows the row actions instead of revealing them on hover. The background color of the row still changes on hover, to make it easier to identify which row you're acting on.
  • I moved the "Add field" and "Add multi-field" buttons to the right so now all actions are grouped together.
  • I converted all of the buttons from text to icons to declutter the UI. I used the "submodule" icon for the "Add multi-field" button because that seemed the most analogous among the EUI icons.
  • I also adjusted the spacing in the field list so that field names would align with icons on other rows.

@mdefazio WDYT?

Simple example

image

Example with a lot of rows

image

@cjcenizal cjcenizal added Feature:Mappings Editor Index mappings editor UI Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more t// labels Dec 20, 2019
@elasticmachine
Copy link
Contributor

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

@mdefazio
Copy link
Contributor

Hi CJ, I will work up a prototype to try some things out as I think there's perhaps a couple ways to tackle the concerns you're addressing in this PR.
One thought off-hand is that all root level text should line up, whether it has an arrow or not. I think we had this in an earlier iteration—without it there's a confusion in the relationship between root items that are next to each other if one has an arrow.
image

Grouping all the actions next to and converting it to only icons makes sense, apart from the multi-field. I wonder how many people will know what that does without a text description? The edit/remove icons are pretty standard so I understand that.

I do think there could be more space between the icons as well so the hit area is a bit more forgiving.
image

Again, i'm not sold yet on removing the hover effect for the actions. Perhaps we only show the 'add' on hover (and maybe dim back the 'edit/remove' unless hovering). However, let me mockup some ideas to give clearer direction.

@cjcenizal cjcenizal changed the title Mappings editor/surface field actions Always show row actions for the fields list in the Mappings Editor Dec 20, 2019
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

History

  • 💔 Build #17022 failed f3724c468f3cecab592c504d61faa00c31def018

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

@sebelga
Copy link
Contributor

sebelga commented Dec 23, 2019

There was also a concern that @pcsanwald mentioned at one of our syncs. The big gap between the field name and the actions on the right, especially on big screens.

I might throw a dumb idea here, but what if we had somehow a tooltip with actions that follows the mouse when hovering each field? So you hover and right there you can act on the field. Just an idea 😊

@cjcenizal cjcenizal force-pushed the mappings-editor/surface-field-actions branch from 83ff029 to 54e807b Compare January 11, 2020 00:16
@cjcenizal
Copy link
Contributor Author

cjcenizal commented Jan 11, 2020

@sebelga I'm going to narrow the purpose of this PR down to just resolving our keyboard-accessibility problems, so for now I'm going to defer new interaction methods like your tooltip idea. Here's where I ended up:

mappings_editor_tabbing

I chose an icon for the "Add multi-field" button that I think is a little clearer (multiple documents):

image

I also experimented with putting the actions on the right, but I think putting them closer to the node name as @mdefazio suggested makes it way easier to associate an action with the node that's affected. I did find myself afraid of accidentally clicking on the delete button because in some situations it doesn't show the confirmation modal, so I tweaked the delete provider logic so that the confirmation modal will show up for all nodes, not just those with children, aliases, or multi-fields.

Here are the icons on the right:

image

And on the left (this is what I settled on):

image

I think this gets us in a good place accessibility-wise, and I think the UX is a step forward from what we currently have, too. I think we can continue to improve this based on @mdefazio's guidance after feature freeze. Do you think this is an acceptable step forward, or do you see any major issues?

In order to mirror these changes, I think we will have to adapt the UI of your search results in the following ways:

  • Reveal-on-hover behavior would be removed
  • Switching from text buttons to icons with tooltips
  • Possibly moving the actions to the left

@cjcenizal
Copy link
Contributor Author

@mdefazio Re your comment from earlier:

One thought off-hand is that all root level text should line up, whether it has an arrow or not. I think we had this in an earlier iteration—without it there's a confusion in the relationship between root items that are next to each other if one has an arrow.

I'll try to update the PR with this, thanks for pointing it out!

@cjcenizal cjcenizal force-pushed the mappings-editor/surface-field-actions branch from 996fb24 to 190856c Compare January 11, 2020 13:18
Copy link
Contributor

@sebelga sebelga left a comment

Choose a reason for hiding this comment

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

@cjcenizal I think we are making progress! Thanks for making those changes. I like to have the button near the field name. For the DeleteProvider, I understand why you did it. It makes me think of something I wanted to work on: a toast system with undo embedded (like gmail has).

Screen Shot 2020-01-13 at 10 04 16

This would free us from those confirmation modal and let a user quickly delete 3 fields without having a modal popping up each time when he knows he wants to delete them.

I also liked the blue background behind the icons from @mdefazio design with the hover emphasis, do you think it would be possible to add those? (From our conversation about Phases II, I am afraid the "I think we can continue to improve this based on @mdefazio's guidance after feature freeze." would never happen).

I personally don't like having the actions visible at all times as it clutters the UI. I think we should understand how EUI makes it work and reproduce it here in the mappings editor.

As an example, a EUI table

Screen Shot 2020-01-13 at 09 49 52

Hover a table row:

Screen Shot 2020-01-13 at 09 49 58

Could you explain the difficulty to do the same in the mappings editor?

@cjcenizal
Copy link
Contributor Author

@elasticmachine merge upstream

@sebelga sebelga self-requested a review January 14, 2020 03:00
Copy link
Contributor

@sebelga sebelga left a comment

Choose a reason for hiding this comment

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

Ok, let's give this UX a try! 👍

@cjcenizal cjcenizal force-pushed the mappings-editor/surface-field-actions branch from 8cbdd71 to 2c310ef Compare January 14, 2020 18:28
@cjcenizal
Copy link
Contributor Author

I brought back the original spacing and use of row icons per @mdefazio's suggestions, except I changed the multi-field row icon to be "documents".

image

@cjcenizal cjcenizal added the release_note:skip Skip the PR/issue when compiling release notes label Jan 14, 2020
@cjcenizal cjcenizal force-pushed the mappings-editor/surface-field-actions branch from 2c310ef to 610d3af Compare January 14, 2020 19:38
@cjcenizal
Copy link
Contributor Author

cjcenizal commented Jan 14, 2020

Search results are now consistent with this approach:

image

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@cjcenizal cjcenizal merged commit 358e87f into elastic:feature/mappings-editor Jan 14, 2020
@cjcenizal cjcenizal deleted the mappings-editor/surface-field-actions branch January 14, 2020 22:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature:Mappings Editor Index mappings editor UI 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//

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants