Skip to content

[Mappings editor] Fix UI and performance issues#54224

Merged
sebelga merged 4 commits intoelastic:feature/mappings-editorfrom
sebelga:mappings-editor/disable-auto-json-editor
Jan 9, 2020
Merged

[Mappings editor] Fix UI and performance issues#54224
sebelga merged 4 commits intoelastic:feature/mappings-editorfrom
sebelga:mappings-editor/disable-auto-json-editor

Conversation

@sebelga
Copy link
Copy Markdown
Contributor

@sebelga sebelga commented Jan 8, 2020 •
edited
Loading

This PR fixes some issues detected when testing the mappings editor

Fixes #54212

Primary button when loading JSON

The "danger" color has been removed.

Screen Shot 2020-01-08 at 13 22 34

"Automagically" switch to the JSON editor when a big mapping is loaded

The behavior has been removed and it will be a conscientious decision by the user to switch to the JSON editor.

Performance when deNormalize the fields

When leaving the mappings editor (changing steps) with a big mapping (the metric beat for example), I noticed the UI not responsive. And indeed, there was a performance bug in the deNormalize() function.

Before (1.73s)

Screen Shot 2020-01-08 at 13 26 42

After (4.58ms):

Screen Shot 2020-01-08 at 13 33 31

@sebelga sebelga added 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// labels Jan 8, 2020
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Jan 8, 2020

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

@sebelga sebelga mentioned this pull request Jan 8, 2020
Closed
@sebelga sebelga mentioned this pull request Jan 8, 2020
Merged
7 tasks
@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Jan 8, 2020

💚 Build Succeeded

History

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

jloleysens
jloleysens approved these changes Jan 8, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@jloleysens jloleysens left a comment

Choose a reason for hiding this comment

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

Tested locally, changes look good to me!

alisonelizabeth
alisonelizabeth approved these changes Jan 8, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@alisonelizabeth alisonelizabeth left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for fixing these issues!

cjcenizal
cjcenizal approved these changes Jan 8, 2020
View reviewed changes
Copy link
Copy Markdown
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.

Tested locally, and I think this looks great! I had a brainstorm while I was testing locally. Why don't we prepopulate the "Load JSON" modal with the existing mappings JSON, and change the language to be "Edit JSON" instead? Doesn't this get us most of the way to the "edit JSON" experience you had in mind, Seb? We also wouldn't need a callout to warn the user that this will replace the existing mappings because that's already implied (#54249).

image

@alisonelizabeth
Copy link
Copy Markdown
Contributor

alisonelizabeth commented Jan 8, 2020

Why don't we prepopulate the "Load JSON" modal with the existing mappings JSON, and change the language to be "Edit JSON" instead?

++ I like this idea a lot.

@sebelga sebelga mentioned this pull request Jan 9, 2020
Merged
@sebelga
Copy link
Copy Markdown
Contributor Author

sebelga commented Jan 9, 2020 •
edited
Loading

Wawww 3 reviewers for this PR... I knew that a +4-9 LOC PR gets more attention 😄 Thanks @jloleysens @cjcenizal @alisonelizabeth

Why don't we prepopulate the "Load JSON" modal with the existing mappings JSON, and change the language to be "Edit JSON" instead?

In that case, I would prefer to have the JsonEditor and get rid of the modal 😊

Your suggestion (having the edit JSON functionality in the modal) implies that we have to validate that there aren't any errors in the form(s) (e.g. that thee _meta JSON is valid). If it's not, we need to clearly indicate to the user why the modal does not open so we have a new UX problem.

At this stage, it is simpler to add a call-out at the top of the load JSON warning the user that this will replace the current mappings and change the confirm button label with "Load and Replace" (The JsonEditor is not yet guaranteed to land for FF).

Happy to have a second thought later on this.

@sebelga sebelga merged commit ebb183b into elastic:feature/mappings-editor Jan 9, 2020
@sebelga sebelga deleted the mappings-editor/disable-auto-json-editor branch January 9, 2020 08:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alisonelizabeth alisonelizabeth alisonelizabeth approved these changes

@jloleysens jloleysens jloleysens approved these changes

+1 more reviewer

@cjcenizal cjcenizal cjcenizal approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@sebelga @elasticmachine @kibanamachine @alisonelizabeth @cjcenizal @jloleysens