Skip to content
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

[Mappings editor] Add UI/UX to mappings core #48876

Merged

Conversation

sebelga
Copy link
Contributor

@sebelga sebelga commented Oct 22, 2019

This PR adds the stylings + improvement on the UX to the Core of the mappings editor. It contains all the suggestions from @cjcenizal in #47335

Screen Shot 2019-10-22 at 11 05 03

Screen Shot 2019-10-22 at 10 49 34

Screen Shot 2019-10-22 at 10 48 43

Screen Shot 2019-10-22 at 11 05 29

Child field vs Multi-field

A child field is a field that belongs to an object, it is one of the properties of an object or nested type. A multi-field is different. It is not a property of the field. It is the same field but indexed differently (e.g. a text field can also be indexed as keyword for aggregation or a text field can be indexed as another text type but with a different analyzer).

To make the difference clear to the user, we worked with @mdefazio to have a different UI when the field is a multi-field vs if it is a property of an object (toggle on the left of the field vs having to click on the count of multi-fields to expand the fields. Also, the multi-field has a "link" icon on its left instead of a "L" bullet list, and multi-fields are separated by dash instead of a solid line.

The current implementation is not the final design from Michael, which is this one:

image

But, as the <EuiBadge /> does not allow a counter, we want to validate first if the UX makes sense before spending time extending the badge component.

What to test

  • When we fill the "name" in the create form, clicking outside sends the form and adds the field to the document
  • If no "name" has been introduced in the field, clicking outside the form closes the create field form. Unless there are no fields added in the document. In that case, the form is always visible. Clicking "next" should not throw an error.
  • When adding or editing a multi-field, the object and nested type should not be available in the select dropdown as those are not valid types for multi-field.
  • We should not be able to nest multi-field. So, a "text" multi-field should not have the "Add multi-field" button visible when hovering.
  • If we delete a field that either has child fields or multi-fields, a modal should open to confirm the deletion.
  • If we change the "type" of field from "text" to anything else but "keyword" and if the field has multi-fields, a modal should open to confirm the type change. And vice versa (from "keyword" to anything else but "text").
  • If we change the "type" of the field from "object" to anything else but "nested" and if the field has child fields, a modal should open to confirm the type change. And vice versa (from "nested" to anything else but "object").
  • When editing a field, the field should be highlighted in the main view and the full path to the field should appear in the flyout header.

When there aren't any fields defined on the document, we show by default the "CreateField" component to add a field.
@sebelga sebelga added the release_note:skip Skip the PR/issue when compiling release notes label Oct 22, 2019
@sebelga sebelga requested a review from mdefazio October 22, 2019 10:09
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@mdefazio
Copy link
Contributor

mdefazio commented Oct 22, 2019

Looks good, just a few small SCSS tweaks:

  • There are a few places in the ul.tree that uses 'em's instead of EUI variables.
  • I think changing the dark border (which was in the mockup, sorry) to $euiBorderThin might make sense
  • Perhaps we change the background color for the create wrapper to $euiColorLightestShade
  • The flyout might need to be wider, or we may need to look at a different model for handling the type + subtype dropdowns in the edit view.

One larger change would be to keep the arrows on the left for collapse/expand for both nested children and multi-fields. Then we are keeping the same patter for showing and hiding. Could perhaps also try keeping the space for the arrow by default so there's not a jump in indentation when adding a child.

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

Left some non-blocker comments - have to test locally still. For some reason I'm getting a OOM state when starting Kibana locally on this branch.

Comment on lines +191 to +195
<FormDataProvider pathsToWatch="type">
{({ type }) => {
return <EuiFlexItem grow={false}>{renderFormFields(type)}</EuiFlexItem>;
}}
</FormDataProvider>
Copy link
Contributor

Choose a reason for hiding this comment

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

Very elegant!


// We _also_ need to make a copy of the parent "childFields"
// array to force a re-render in the view.
if (parentField.parentId) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not entirely clear on why this is need - as I understand it the byId object is a flat mapping on the tree so not sure what we gain by copying the array on another id on the same object and how this change result in the update?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, the logic had some complexity because of some memoization down the component tree. I updated the logic and removed the need of those non-intuitive array updates. Thanks for pointing this!

@elasticmachine
Copy link
Contributor

💔 Build Failed

Copy link
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 all the points mentioned in description and everything works as expected.

The only UX point I'd like to add is perhaps showing the shortcuts on hover to open another field editor (collapsing the current one open the other one). So on hover of the field below in the picture that edit controls on the right would show. Just a suggestion.

Screenshot 2019-10-23 at 15 43 08

@sebelga
Copy link
Contributor Author

sebelga commented Oct 23, 2019

Thanks for the review @mdefazio and @jloleysens ! I have made all your recommended changes.

I will merge this PR to the feature branch so I can update my current working branch. Would you mind having a look at the feature branch and confirm the design and behaviour is OK?

The only UX point I'd like to add is perhaps showing the shortcuts on hover to open another field editor (collapsing the current one open the other one). So on hover of the field below in the picture that edit controls on the right would show. Just a suggestion.

It was a design decision to not allow adding child or multi-field meanwhile we are in "create" or "edit" mode. But that was before the "ClickOutside" behaviour.
As this change might add some complexity, I prefer to defer it to later and add it as an improvement . 👍

@sebelga sebelga merged commit 1ffc5d5 into elastic:feature/mappings-editor Oct 23, 2019
@sebelga sebelga deleted the feature/mappings-editor-add-ui branch October 23, 2019 15:37
@elasticmachine
Copy link
Contributor

💔 Build Failed

@mdefazio
Copy link
Contributor

This looks good to me!

@snide
Copy link
Contributor

snide commented Oct 23, 2019

Hey @sebelga I see some CSS stuff in here that could be improved. I'll give you a PR to review, but some pointers from the sass docs.

  1. Follow the naming schema rules.
  2. Don't use pixel values 4px = $euiSizeXS...etc
  3. prefix your component to avoid conflicts in Kibana
  4. Don't use naked selectors like div, ul...etc.

An example of why this can be important. You have a naked ul.tree in use here. Who knows who else is using a class name like that. There's a lot of devs in kibana, and unfortunately kibana's css spills globally.

@sebelga
Copy link
Contributor Author

sebelga commented Oct 24, 2019

Thanks for the review @snide

I kind of followed all the recommendations you mention aside from 1 exception here and there it seems 😊

Follow the naming schema rules.

Are you referring to using camel case for the class names?

@mdefazio
Copy link
Contributor

Yes, I believe so... appBlockItem__eleMent or appBlockItem--modiFier

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 v7.6.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants