-
Notifications
You must be signed in to change notification settings - Fork 860
[Visual Refresh] Update form layout append/prepend API
#9014
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
[Visual Refresh] Update form layout append/prepend API
#9014
Conversation
|
This PR contains breaking changes. The opener of this pull request is asked to perform the following due diligence steps below, to assist EUI in our next Kibana upgrade:
|
|
ℹ️ Initial design feedback:
|
e870cba to
6dc14e7
Compare
|
ℹ️ I rebased this branch and EDIT: The branch was rebased again with the reinstated feature branch |
6dc14e7 to
dde1d94
Compare
- updates EuiFormControlLayout by adding a context to share props - updates EuiFormLabel to support visual-only renders as span element
dde1d94 to
8c1081c
Compare
- aligns styling with EuiFormAppendPrepend styles
| {children} | ||
| </legend> | ||
| ); | ||
| } else if (type === 'span') { |
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'm wondering if we also want to update this to else if (type === 'span' || !htmlFor) to prevent rendering a <label> when the input linking is not applied as a <label> would expect a relation to a linked element.
This would likely affect more tests on Kibana side, as I'm not sure how diligently the inputId is applied.
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 it makes sense to check for !htmlFor. We're already introducing changes that will likely affect Kibana tests, so as long as this wouldn't result in breaking unreasonable number of them that would need manual updates, I'd say we should go with it
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.
Considering this will already be a breaking change and we'll have a QA phase, I think it should be reasonable to include the update in the overall append/prepend update feature. We'll merge this into a feature branch anyway, worst case we can revert if it causes to much trouble.
Added in 4da2bbe.
I also updated the docs to try and explain the different scenarios more clearly.
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.
ℹ️ Fyi, the potential test failures I'd expect here are:
- snapshots (
<span>instead of<label>element is rendered) - selectors not available if a test checks for a
labelrole
As this is on the EuiFormLabel component, it could happen across all of Kibana, it's not directly related to the Append/Prepend change.
...ages/eui/src/components/form/form_control_layout/append_prepend/form_append_prepend.test.tsx
Outdated
Show resolved
Hide resolved
packages/eui/src/components/form/form_control_layout/form_control_layout.tsx
Show resolved
Hide resolved
- clarifies the different render outputs and ways to properly label append/prepend
377d296 to
0e50bb0
Compare
|
Thank you for addressing my comments! Code changes look good, I'm currently going through QA steps to verify everything. I opened #9293 to have a testing environment for the feature branch |
💚 Build SucceededHistory
cc @mgadewoll |
💚 Build Succeeded
History
cc @mgadewoll |
tkajtoch
left a comment
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.
Code changes look great, QA passed. LGTM
fee46b7
into
elastic:feat/visual-refresh-append-prepend-updates
Summary
closes https://github.com/elastic/eui-private/issues/351
Important
This PR merges into a feature branch.
This PR implements two new components:
EuiFormAppendandEuiFormPrepend.These components are meant to be passed into form layout
appendandprependslots to ensure a cohesive layout and styling.These two components are meant to replace the current generic slot API, but for the initial introduction this implementation ensures a backwards compatible usage to make the update process smoother. This currently means that the previous usages of passing other components to the
append/prependslots still works and will results in mostly identical visual output.Once consumers are updated fully, the idea is to remove the additional style rules that ensure backwards compatibility and rely on usages of
EuiFormAppendandEuiFormPrependonly. Other usages would then result in unexpected visual output and indicate misusage.Changes
EuiFormAppendandEuiFormPrependcomponents (components, docs, tests)type="span"onEuiFormLabelto support using visual-only form labelsEuiFormLabelto render aspanif nohtmlForis passedEuiFormControlLayoutto useEuiFormAppendandEuiFormPrependEuiFormAppend/EuiFormPrependas needed (EuiAutoRefresh,EuiColorPicker)EuiQuickSelectPopoverinEuiSuperDatePickerto useEuiFormPrepend: This results in more restrictedquickSelectButtonPropsas they reflectEuiFormPrependinstead of genericEuiButtonEmptyprops.EuiColorPickerto ensureidis correctly passed onto the internalEuiFormControlLayoutWhy are we making this change?
💅 UI consistency: This update is part of the Visual Refresh project and it's purpose is to ensure a cohesive visual output for form layout append/prepend elements.
Screenshots #
Previous "generic slot" usage
New EuiFormAppend/Prepend usage
EuiAutoRefresh
EuiSuperDatePicker
EuiColorPicker
Impact to users
quickSelectButtonPropsonEuiSuperDatePickermaybe cause breaking changes as it reflects a more restricted prop type forEuiFormPrependinstead ofEuiButtonEmpty.🟢 There are no usages of
quickSelectButtonPropsonEuiSuperDatePickerorEuiQuickSelectPopoverdirectly in Kibana or Cloud.ℹ️ The backwards compatibility of the changes ensures that existing Kibana usages work as expected.
We do however want to update simple Kibana usages to use
EuiFormAppend/EuiFormPrependalready to drive adoption. This will be done as an additional step and linked here once available.Deployment
💻 Kibana instance (credentials)
ℹ️ This instance currently does not have any usage updates yet.
QA
EuiFormAppend/EuiFormPrependcomponents and their usage inappend/prependusage and ensure it matches design specs (figma, figma)append/prependbetween feature branch and stagingverify there is no regression forappend/prependin Amsterdam (feature branch, staging)EuiSuperDatePicker's quick select button looks/works as expected (feature branch, staging)EuiAutoRefreshlooks according to new design specs (staging)EuiColorPickerhas no regression (feature branch, staging)General checklist
@defaultif default values are missing) and playground togglesChecked Code Sandbox works for any docs examplesIf applicable, file an issue to update EUI's Figma library with any corresponding UI changes. (This is an internal repo, if you are external to Elastic, ask a maintainer to submit this request)