Skip to content

[Mappings editor]: Addons and bug fixes#51018

Merged
sebelga merged 35 commits intoelastic:feature/mappings-editorfrom
sebelga:fix/mappings-editor-bugs
Nov 19, 2019
Merged

[Mappings editor]: Addons and bug fixes#51018
sebelga merged 35 commits intoelastic:feature/mappings-editorfrom
sebelga:fix/mappings-editor-bugs

Conversation

@sebelga
Copy link
Contributor

@sebelga sebelga commented Nov 19, 2019

This PR adds some improvement to the Mappings editor and fixes a few bugs.

Improvements

Screen Shot 2019-11-19 at 08 34 14

Screen Shot 2019-11-19 at 09 12 29

  • The mappings editor can now read the index settings and expose possible custom analyzer defined by the user under the Settings tab. If no custom analyzer has been defined, the "Custom analyzer" option in the dropdown will not appear.

Screen Shot 2019-11-18 at 10 18 48

Screen Shot 2019-11-19 at 13 10 23

  • Template name must be in lowercase. A new validation has been added for it on the field.

Fixed bugs

  • When editing a field, the value in the "Advanced settings" are now kept after closing/opening
  • Many small updates on the form lib to make it even more robust 🎉

Refactor

Screen Shot 2019-11-16 at 04 52 13

@sebelga sebelga added Feature:Index Management Index and index templates 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// v7.6.0 v8.0.0 labels Nov 19, 2019
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

💔 Build Failed

Copy link
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.

@sebelga thanks for these fixes!

I did a light code review, overall LGTM.

While testing, I did notice that when I enabled boost and null value, but didn't provide a value, there was no validation and the UI treated it as if I did not enable it when I saved. (This might have been a previous bug, but just happened to notice while testing that the "Advanced settings" were saved when opened/closed).

Screen Shot 2019-11-19 at 9 42 01 AM

Also, I was seeing strange behavior with the error callout for Set position increment gap. I could only produce in edit mode, but the error callout would flash for a second and then disappear when toggling the switch. It seemed to be working as expected when creating a new template.


I like that the description text is now a part of the select options. However, it does make for a lot more scrolling for parameters that have a lot of options. It took me a few minutes to figure out why my custom analyzer wasn't showing. I was thrown off that there was still the "Add custom" link, but then realized there was also a "Custom analyzer" option at the very bottom of the list. Maybe we can get some feedback on this and revisit if there's time 😄

formFieldPath === undefined ? (
<EuiSwitch checked={isContentVisible} onChange={onToggle} data-test-subj="input" />
// TODO: Ask EUI why the "label" is a required prop since last update
<EuiSwitch label="" checked={isContentVisible} onChange={onToggle} data-test-subj="input" />
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should still define a label here, but set showLabel={false} for screen readers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I will add it

@sebelga
Copy link
Contributor Author

sebelga commented Nov 19, 2019

Thanks for the review @alisonelizabeth !! I will merge this so you have time to merge master and resolve the conflicts (and hopefully get the feature branch PR turn green! 😊 ). I will address any upcoming feedback on my next branch.

While testing, I did notice that when I enabled boost and null value, but didn't provide a value, there was no validation and the UI treated it as if I did not enable it when I saved. (This might have been a previous bug, but just happened to notice while testing that the "Advanced settings" were saved when opened/closed).

It seems to be the expected behavior. If you don't specify anything in the field, the user probably bypassed the set null value. He can always come back and set it if he needs to, but it is not an error per se IMO.

Also, I was seeing strange behavior with the error callout for Set position increment gap. I could only produce in edit mode, but the error callout would flash for a second and then disappear when toggling the switch. It seemed to be working as expected when creating a new template.

I will try to reproduce it, I haven't noticed anything on my local but I'll check. Thanks 👍

I like that the description text is now a part of the select options. However, it does make for a lot more scrolling for parameters that have a lot of options.

Yeah, more scrolling but I guess we better educate the users on what option does what. Let's see what stakeholders think.

I was thrown off that there was still the "Add custom" link

We need to figure out if there is still the need to add a custom analyzer that would come from a plugin for e.g. @jethr0null do you think you could find out?

@sebelga sebelga merged commit ac682ed into elastic:feature/mappings-editor Nov 19, 2019
@sebelga sebelga deleted the fix/mappings-editor-bugs branch November 19, 2019 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature:Index Management Index and index templates 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// v7.6.0 v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants