[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.
💭 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.
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.
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.
ℹ️ 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.
Code changes look great, QA passed. LGTM
fee46b7
into
elastic:feat/visual-refresh-append-prepend-updates
## Dependency updates - `@elastic/eui` - v112.3.0 ⏩ v113.0.0 - `@elastic/eui-theme-borealis` - v5.4.0 ⏩ v6.0.0 - `@elastic/eslint-plugin-eui` - v2.8.0 ⏩ v2.9.0 --- ## Changes 1. EUI component update: API and Design updates to form layout append/prepend by using the new components `EuiFormPrepend` and `EuiFormAppend` (EUI [#9308](elastic/eui#9308)) >[!important] ❗ This upgrade PR includes all changes previously made in the specific QA PR #248805. Please see the description in that PR for detailed information about the specific changes. 2. Additional changes related to the form layout changes >[!note] 🔗 Please see the [section "Additional changes"](#248805 (comment)) in #248805 for an overview of additional changes. 3. Removed token update: Replaced `ghost` and `ink` token usages with `textInk` or `plainDark` and `textGhost` or `plainLight` respectively (EUI [#9379](elastic/eui#9379)) ## Package updates ### `@elastic/eui` [v113.0.0](https://github.com/elastic/eui/releases/tag/v113.0.0) - Updated `EuiFlyout` manager to close all flyouts when a parent flyout is closed. ([#9378](elastic/eui#9378)) - Added `textInk` and `textGhost` color tokens for text and icon colors that should always remain dark or light regardless of color mode. ([#9379](elastic/eui#9379)) - Added `EuiFormAppend` and `EuiFormPrepend` components ([#9014](elastic/eui#9014)) - Added support for `type="span"` on `EuiFormLabel` for visual-only form labels ([#9014](elastic/eui#9014)) - Updated `EuiFormLabel` to render a `span` if no `htmlFor` is passed ([#9014](elastic/eui#9014)) - Updated `EuiFormControlLayout` to use `EuiFormAppend` and `EuiFormPrepend` ([#9014](elastic/eui#9014)) - Updated `EuiAutoRefresh` and `EuiColorPicker` to use `EuiFormPrepend` ([#9014](elastic/eui#9014)) - Updated `EuiFormAppend`/`EuiFormPrepend` styling ([#9305](elastic/eui#9305)) - Updated `EuiFormAppend`/`EuiFormPrepend` to inherit `isDisabled` state from `EuiFormControlLayout` ([#9305](elastic/eui#9305)) - Updated `EuiFormControlLayout` hover, disabled and readonly styling ([#9305](elastic/eui#9305)) - Updated `EuiFormControlButton` to inherit `isDisabled`, `readOnly` and `isInvalid` states from `EuiFormControlLayout` ([#9305](elastic/eui#9305)) - Added `iconSide` prop on `EuiDatePickerRange` ([#9305](elastic/eui#9305)) - Updated `EuiSuperDatePicker` valid state styling ([#9305](elastic/eui#9305)) - Removed background color transition on `EuiButtonEmpty` (other button variants don't have a transition anymore either) ([#9305](elastic/eui#9305)) - Added `isLoading` prop on `EuiFormControlButton` ([#9328](elastic/eui#9328)) - Updated paddings for `EuiButton`, `EuiButtonEmpty`, `EuiFilterButton` ([#8948](elastic/eui#8948)) - Updated paddings for `append`/`prepend` on `EuiFormControlLayout` ([#8948](elastic/eui#8948)) - Added optional `scrollContainerRef` prop to `EuiFlyoutBody` for accessing the flyout's internal scroll container. ([#9373](elastic/eui#9373)) **Bug fixes** - Updated `EuiColorPicker` to ensure `id` is correctly passed onto the internal `EuiFormControlLayout` ([#9014](elastic/eui#9014)) **Breaking changes** - Removed `ink` and `ghost` theme tokens. Use `textInk` / `textGhost` for text and icon colors or `plainDark` /`plainLight` for non-text use cases. ([#9379](elastic/eui#9379)) - Updated `EuiQuickSelectPopover` in `EuiSuperDatePicker` to use `EuiFormPrepend`. This results in more restricted `buttonProps` as they reflect `EuiFormPrepend` instead of generic `EuiButtonEmpty` props. ([#9014](elastic/eui#9014)) - Removed `components.superDatePickerBackgroundSuccees` token ([#9305](elastic/eui#9305)) ### `@elastic/eui-theme-borealis` v6.0.0 - Added `textInk` and `textGhost` color tokens for text and icon colors that should always remain dark or light regardless of color mode. ([#9379](elastic/eui#9379)) ### `@elastic/eslint-plugin-eui` v2.9.0 - Prevented `badge-accessibility-rules` rule autofix from duplicating `aria-hidden` attributes. ([#9366](elastic/eui#9366)) - Skip `badge-accessibility-rules` rule validation when a spread operator is used in a component. ([#9366](elastic/eui#9366)) --------- Co-authored-by: Weronika Olejniczak <weronika.olejniczak@elastic.co> Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
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)