-
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
[Controls] Field first control creation #131461
[Controls] Field first control creation #131461
Conversation
5df9bae
to
afbbb78
Compare
…-ref HEAD~1..HEAD --fix'
35c6160
to
826b447
Compare
0852350
to
a4d516a
Compare
Pinging @elastic/kibana-presentation (Team:Presentation) |
a4d516a
to
e9e175e
Compare
e9e175e
to
2e7d2ef
Compare
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.
Overall, this is pretty much exactly the way we spoke about it, but with a really elegant implementation. I left a few tiny nits, and one change for readability / efficiency with the react state management.
I tested this locally as well and everything works great!
@@ -56,6 +64,10 @@ export const ControlGroupStrings = { | |||
i18n.translate('controls.controlGroup.manageControl.widthInputTitle', { | |||
defaultMessage: 'Control size', | |||
}), | |||
getControlSettingsTitle: () => | |||
i18n.translate('controls.controlGroup.manageControl.controlSettingsTitle', { | |||
defaultMessage: 'Control settings', |
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.
I wonder if we should use the friendly name of the selected Control Type here, so it would read Options List settings
or Range slider settings
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.
Great point! @andreadelrio what are your thoughts?
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.
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.
@andreadelrio Yeah, it seemed to me like grouping the type-specific control settings with the Minimum width
settings was confusing - so either the type-specific settings should go above the Minimum width
(which would cause shifting when switching between Options list
and Range slider
fields) or I think these type-specific settings should have their own label.
What do you think, though?
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.
I think we could either group them under something like Additional settings? or give each toggle its own title so something like:
- Multiple selections
Allow multiple selections in dropdown
- Timeout
Run past timeout
What do you think @KOTungseth ?
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.
I like Additional settings
but definitely interested in hearing @KOTungseth's thoughts! :)
src/plugins/controls/public/control_group/editor/control_editor.tsx
Outdated
Show resolved
Hide resolved
src/plugins/controls/public/control_types/options_list/options_list_editor_options.tsx
Show resolved
Hide resolved
src/plugins/controls/public/control_types/options_list/options_list_embeddable.tsx
Show resolved
Hide resolved
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.
I actually thought about that, but ended up going with not for now just to make it super transparent on how the control types are being controlled. @ThomThomson What do you think?
@andreadelrio I added |
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.
Thanks for making those changes. 🎉 LGTM
Just for the sake of a public follow up, these are the final changes that were made with @andreadelrio's feedback: |
💚 Build SucceededMetrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
History
To update your PR or re-run it, just comment with: cc @Heenawter |
Closes #129791
Summary
In the old way of creating controls, users had to first select the type of control they wanted to create, which would then filter the list of available fields to those that were supported - this is what we will call type-first control creation. However, in our user testing, we found that users were more likely to have a field-first control creation in mind - that is, they knew what field they wanted to create a control for, but they didn't know which control type to pick and they were confused about why their desired field wasn't showing up in the field list.
Field first creation
This PR introduces a new field-first control approach - when creating a control, the user simply needs to select a field and the corresponding supported control type will be automatically picked for them. Note that, because only options lists and range sliders are supported and they have no crossover in field types, there is only ever one available control type for each field - however, if more control types are introduced in the future that allow for an overlap in supported field types, we will need to allow users to select one of the supported control types as a secondary step.
2022-05-09_FieldFirstCreation.mp4
Field first replace
As a benefit, when combined with the ability to change the types of controls that was introduced in this PR, the overall user experience for both creating and editing existing controls is significantly improved.
2022-05-10_FieldFirstReplace.mp4
Flaky Test Runner
Checklist
Delete any items that are not applicable to this PR.
For maintainers