-
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
[Composable template] Preview composite template #72598
[Composable template] Preview composite template #72598
Conversation
Pinging @elastic/es-ui (Team:Elasticsearch UI) |
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.
@sebelga Great work!
I think the preview tab is going to be super helpful to users and really helps solidify how component templates and templates fit together 🤓!
Overall I only found one bug that I think we should look into, but otherwise I think this is good to go!
UI/UX
- I noticed the flyout has an "Update" button, but it didn't look I needed to ever press it. Is there a case in which you think this button will be needed? If not, perhaps we can remove it.
- As mentioned in the code comments, it would be good to get es-docs@ to take a look at the new copy.
Bugs
- When editing a mapping in the mappings editor, selecting "Field type" and pressing backspace resulted in the UI crashing with:
...lugins/es_ui_shared/__packages_do_not_import__/flyout_multi_content/flyout_multi_content.tsx
Outdated
Show resolved
Hide resolved
...lugins/es_ui_shared/__packages_do_not_import__/flyout_multi_content/flyout_multi_content.tsx
Outdated
Show resolved
Hide resolved
...lugins/es_ui_shared/__packages_do_not_import__/flyout_multi_content/flyout_multi_content.tsx
Outdated
Show resolved
Hide resolved
...lugins/es_ui_shared/__packages_do_not_import__/flyout_multi_content/flyout_multi_content.tsx
Outdated
Show resolved
Hide resolved
...lugins/es_ui_shared/__packages_do_not_import__/flyout_multi_content/flyout_multi_content.tsx
Outdated
Show resolved
Hide resolved
...onents/mappings_editor/components/document_fields/fields/edit_field/edit_field_container.tsx
Show resolved
Hide resolved
...onents/mappings_editor/components/document_fields/fields/edit_field/edit_field_container.tsx
Show resolved
Hide resolved
...k/plugins/index_management/public/application/components/mappings_editor/mappings_editor.tsx
Outdated
Show resolved
Hide resolved
...k/plugins/index_management/public/application/components/mappings_editor/mappings_editor.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/index_management/public/application/components/template_form/template_form.tsx
Outdated
Show resolved
Hide resolved
@sebelga I love how we're making the interaction between index templates and component templates more transparent to users with this feature! At the same time I have some UX concerns. Can you help me understand a couple things? What's the thinking behind exposing the user to the entire set of composed configurations at every step? For example, let's say I'm looking at the component templates step: At this step, I'm asking myself, "How do these component templates combine?" In the current UX, the preview shows the final output, so if a configuration from a later step overrides the configuration of the component templates I see here, then I will be confused about the connection between the information on the screen. If we show the user just the result of the component templates they've selected (which can be implemented with a special simulate call) then we can avoid this confusion. I'd assume the user would go to the Review step any time they want to see the end result. Similarly, let's say I'm editing settings: At this point the question I'm asking myself is, "How do these settings override those of the component templates?" But if the preview is full of mappings information, I need to hunt through the preview to find the information I need. This is really frustrating when the object is large. This UX pain point applies to the mappings and aliases tabs too. Did you consider a UX in which we only show the user pertinent information at every step? I think there are some interesting UX benefits we could provide if we explored this approach. For example, we could render errors that are specific to a step by building step-specific simulate requests that exclude configurations from the other steps. I'm picturing the error below rendered next to the Settings JSON editor: |
Co-authored-by: Jean-Louis Leysens <jloleysens@gmail.com>
Thanks for the review @jloleysens and @cjcenizal !
I haven't noticed that, I will check 👍 Do you remember in which flyout this occurs? I guess it is not the mappings editor "edit field" one 😊
I will look into that. I wonder if that is in master already or not... I (almost) never use backpack! 😊 @cjcenizal I hear your concerns. I personally prefer not to think for our users. Some might want to see the full template, some might be bothered by too much information. I don't think that we are (yet) in a position to decide for them and narrow what they see and don't see. I prefer to leave them the choice of that decision. I suggest to have either a filter dropdown select that lets the user do the narrowing himself with the following options: Preview
or the same but in a horizontal line of checkboxes (if they fit in one line) Would that help alleviate your concerns? |
@jloleysens This is needed if you have the flyout open and for example you add a component template or you edit the index settings. At that moment you need to click the "Update" button to see the changes reflected. |
@sebelga I think a UI component that lets you select which part of the preview you want to see is a great solution! To the discussion about the "Update" button, I noticed that there isn't any feedback about whether it needs to be pressed or not. This would contribute to the confusion JL experienced (I experienced this too). I've seen this solved in other apps by disabling the button and showing some kind of message, e.g. "Up to date", when the preview is up-to-date. I also suggest setting |
@jloleysens I've addressed all your feedback, do you mind having another look? Cheers! |
@cjcenizal For the improved UX of the filtering of the preview and the "Update" button I will address them in a separate PR. I want to make sure that this PR gets merged today. For the "Update" button improvement this will require some work as currently the mappings editor sends change update on each keystroke of the forms. Here we only want to be alerted when a new property has been added or deleted. So the onChange handler has to be revisited. |
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.
@sebelga Thanks for addressing those points! I read the code for the bug fix (not allowing empty types) and am satisfied that it is fixed - did not re-test locally.
@elasticmachine merge upstream |
💚 Build SucceededBuild metrics@kbn/optimizer bundle module count
async chunks size
page load bundle size
History
To update your PR or re-run it, just comment with: |
* master: (111 commits) Remove flaky note from gauge tests (elastic#73240) Convert functional vega tests to ts and unskip tests (elastic#72238) [Graph] Unskip graph tests (elastic#72291) Add default Elasticsearch credentials to docs (elastic#72617) [APM] Read body from indicesStats in upload-telemetry-data (elastic#72732) The directory in the command was missing the /generated directory and would cause all definitions to be regenerated in the wrong place. (elastic#72766) [KP] use new ES client in SO service (elastic#72289) [Security Solution][Exceptions] Prevents value list entries from co-existing with non value list entries (elastic#72995) Return EUI CSS to Shareable Runtime (elastic#72990) Removed useless karma test (elastic#73190) [INGEST_MANAGER] Make package config name blank for endpoint on Package Config create (elastic#73082) [Ingest Manager] Support DEGRADED state in fleet agent event (elastic#73104) [Security Solution][Detections] Change detections breadcrumb title (elastic#73059) [ML] Fixing unnecessary deleting job polling (elastic#73087) [ML] Fixing recognizer wizard create job button (elastic#73025) [Composable template] Preview composite template (elastic#72598) [Uptime] Use manual intervals for ping histogram (elastic#72928) [Security Solution][Endpoint] Task/policy save modal text change, remove duplicate policy details text (elastic#73130) [Maps] fix tile layer attibution text and attribution link validation errors (elastic#73160) skip ingest pipeline api tests ...
This PR adds the "Preview template" functionality to composable templates.
The user can preview the final composite template in 3 places
How to test
If you want to test the back and forth between flyouts you might need to set the zoom level to 50% in order to see the "Preview template" button all the time. See the image below
Notes for the mappings editor
I had to lift the
<StateProvider />
up and wrap the app with it so we consume its component anywhere in the app. For this PR we needed to be able to open the<EditField />
flyout content inside the global Flyout.This change implied a big refactor, with logic that got moved in new files but that hasn't been touched.
Note for reviewer
I would recommend to follow the commits history for the review as it builds incrementally the feature and changes made.
Release notes
We have added the possibility to preview the final composite of a composable template. The user will be able to see this preview from the creation or editing wizard flow, or when looking at the details of a composable template.
TODO
In follow-up PRs I will
FlyoutMultiContent
FlyoutMultiContent