[Emotion] Convert EuiFormControlLayoutDelimited (2/3) #7957
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- each input can use `controlOnly` to reduce number of layout wrappers rendered + remove direct `euiFieldNumber` className
- allows us to write us less repeated CSS
- delimited is just there for classNames, group styles already takes care of it. disabled/readonly can just inherit form styles
- underline needs to be on the children wrapper and not on the full parent - extend `euiFormControlDefaultShadow` to allow setting either just the box-shadow or the background-gradient
- JS context is generally more reliable than wildcard CSS, and consumers should generally prefer to use EUI components rather than not (or set their own styles if they're using raw HTML inputs) + fix EuiFormControlLayoutDelimited not inheriting fullWidth from form context
- doesn't have theme tokens and can just be a static const
+ Fix `cloneElementWithCss()` throwing an error if used twice without a `key` - remove unnecessary `!important`s - no longer needed with flattened Emotion styles - remove unnecessary `min-width: 0` (no longer an issue on Firefox?)
…FormControlLayoutDelimited` className - move inline styles to a nested obj for greater clarity - keeping the nesting now for laziness / since we might be rewriting date picker - `background-color` has some order shenanigans :T hopefully it's commented enough
+ remove unnecessary group/delimiter styles
…essary specificity - !important and other extra specificity comments can be removed - border radius is now completely handled by form control layouts and is no longer needed
+ remove unnecessary margin unset - not seeing it being used
…ed to always appending - this was otherwise causing a `shouldRenderWithCustomStyles` test failure in EuiDualRange
cee-chen
force-pushed
the
emotion/form-layout-2
branch
from
4027cc3
to
25e2db7
Compare
cee-chen
changed the title
mgadewoll
reviewed
Aug 13, 2024
packages/eui/src/components/form/form_control_layout/form_control_layout_delimited.styles.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
[adding here for loose connection]
I notice that borders of append/prepend don't align correctly with the form control wrapper anymore. Is that expected?
Screen.Recording.2024-08-13.at.09.43.45.mov
There was a problem hiding this comment.
😭 Thanks for pointing this out!
There was a problem hiding this comment.
Should be fixed now in 4216c21
(edit: force pushed, forgot to update VRT)
.../.loki/reference/chrome_desktop_Forms_EuiForm_EuiFormControlLayoutDelimited_Kitchen_Sink.png
Outdated
Show resolved
Hide resolved
…donly states to start/end controls
- Fix `disabled`+`invalid` to hide invalid underline (transparent) - tweak `euiFormControlDefaultShadow` to allow more granular CSS - it's possible I should just rewrite this whole mixin at this point? :| - Update delimited styles to smoothly transition background color for disabled/readonly bgs (to match the inputs)
- have to use border and not box-shadow/padding for `overflow: hidden` to work correctly - I think I missed this/f'd this up in the amsterdam overrides
cee-chen
force-pushed
the
emotion/form-layout-2
branch
from
5aef227
to
4216c21
Compare
💚 Build Succeeded
History
|
mgadewoll
approved these changes
Aug 14, 2024
Ikuni17
pushed a commit
to elastic/kibana
that referenced
this pull request
Aug 31, 2024
`v95.7.0` ⏩ `v95.9.0` > [!CAUTION] > This PR contains the final set of Emotion conversions for all EuiForm components. > If your plugin contains any custom CSS/styling to **EuiFormRow, EuiFormControlLayout, EuiCheckbox, EuiRadio, or EuiSwitch**,⚠️ make sure to QA your UI to ensure no visual regressions have occurred!⚠️ --- ## [`v95.9.0`](https://github.com/elastic/eui/releases/v95.9.0) - Updated `EuiSearchBar`'s optional `box.schema` prop with a new `recognizedFields` configuration. This allows specifying the phrases that will be parsed as field clauses ([#7960](elastic/eui#7960)) - Updated `EuiIcon` with a new `tokenSemanticText` glyph ([#7971](elastic/eui#7971)) - Added support for TypeScript 5 ([#7980](elastic/eui#7980)) **Bug fixes** - Fixed `EuiSelectableTemplateSitewide` styles when used within a dark-themed `EuiHeader` ([#7977](elastic/eui#7977)) ## [`v95.8.0`](https://github.com/elastic/eui/releases/v95.8.0) - Updated `EuiHeaderLinks`'s mobile menu to set a slight popover padding by default ([#7961](elastic/eui#7961)) - This can be overridden via `popoverProps.panelPaddingSize` if needed. - Updated `EuiHeaderLink` to default to a size of `s` (down from `m`) ([#7961](elastic/eui#7961)) **Accessibility** - Updated the `aria-label` attribute for the `EuiFieldSearch` clear button ([#7970](elastic/eui#7970)) **Bug fixes** - Fixed a visual bug with `<EuiDualRange showInput="inputWithPopover" />` form controls ([#7957](elastic/eui#7957)) **Deprecations** - Deprecated `EuiFormRow`'s `columnCompressedSwitch` display prop. Use `columnCompressed` instead, which will automatically account for child `EuiSwitch`es ([#7968](elastic/eui#7968)) - Deprecated `EuiFormRow`'s `rowCompressed` display prop. Use `row` instead for vertical forms, or `centerCompressed` for inline forms ([#7968](elastic/eui#7968)) - (Styling) Updated `EuiFormRow`'s `hasEmptySpaceLabel` prop to no longer attempt to automatically align its content to a vertical center. Use the `display="center"` prop for that instead ([#7968](elastic/eui#7968)) **CSS-in-JS conversions** - Converted `EuiFormControlLayout` to Emotion ([#7954](elastic/eui#7954)) - Removed `.euiFormControlLayout--*icons` classNames and `--eui-form-control-layout-icons-padding` CSS var. Use `--euiFormControlRightIconsCount` or `--euiFormControlLeftIconsCount` instead - Converted `EuiFormLayoutDelimited` to Emotion ([#7957](elastic/eui#7957)) - Fixed `cloneElementWithCss` throwing an error when used multiple times without a `key` prop ([#7957](elastic/eui#7957)) - Updated `cloneElementWithCss` utility to support a third argument that allows prepending vs. appending the cloned Emotion css className ([#7957](elastic/eui#7957)) - Removed `@euiFormControlLayoutClearIcon` Sass mixin ([#7959](elastic/eui#7959)) - Converted `EuiDescribedFormGroup` to Emotion ([#7964](elastic/eui#7964)) - Converted `EuiForm`, `EuiFormHelpText`, and `EuiFormErrorText` to Emotion ([#7966](elastic/eui#7966)) - Converted `EuiFormLabel` and `EuiFormLegend` to Emotion; Removed `@euiFormLabel` mixin ([#7967](elastic/eui#7967)) - Converted `EuiFormRow` to Emotion ([#7968](elastic/eui#7968)) - Converted `EuiCheckbox` to Emotion ([#7969](elastic/eui#7969)) - Converted `EuiRadio` to Emotion ([#7969](elastic/eui#7969)) - Converted `EuiSwitch` to Emotion ([#7969](elastic/eui#7969)) - Removed the following Sass variables: ([#7969](elastic/eui#7969)) - `$euiFormCustomControlDisabledIconColor` - `$euiFormCustomControlBorderColor` - `$euiRadioSize` - `$euiCheckBoxSize` - `$euiCheckboxBorderRadius` - `$euiSwitchHeight` (and compressed/mini variants) - `$euiSwitchWidth` (and compressed/mini variants) - `$euiSwitchThumbSize` (and compressed/mini variants) - `$euiSwitchIconHeight` - `$euiSwitchOffColor` - Removed the following Sass mixins: ([#7969](elastic/eui#7969)) - `euiIconBackground` - `euiCustomControl` - `euiCustomControlSelected` - `euiCustomControlDisabled` - `euiCustomControlFocused` --------- Co-authored-by: Marta Bondyra <4283304+mbondyra@users.noreply.github.com>
4 tasks
Ikuni17
pushed a commit
to elastic/kibana
that referenced
this pull request
Sep 24, 2024
## Summary This is a follow up to EUI's Emotion conversion of **EuiFormControlLayout/Delimited** (see #190752, elastic/eui#7954, and elastic/eui#7957). > [!note] > Please manually QA your team's affected form control(s) to confirm they still look and behave as expected and are non-broken. The EUI team is not familiar enough with each plugin's setups to pull down and QA this PR ourselves. While QA testing the upgrade, I noticed a few incorrect usages of **EuiFormControlLayout** but wanted to wait until after the upgrade to push out fixes (to prevent delaying the PR further). In general, here is EUI's [recommended usage of the component](https://eui.elastic.co/#/forms/form-controls#form-control-layout): - Where possible, **simply don't use it**. Almost all form controls are **already** automatically wrapped in any EuiFormControlLayout by default, and should accept a large majority of the props that the layout accepts. - If you **must** use it, set the `controlOnly` prop on the child input/control to avoid buggy styling (e.g. duplicate borders). - If you can't do either of the above for any reason (e.g. missing prop support), reach out to the EUI team to ask for your UX as a feature request! ### Checklist Delete any items that are not applicable to this PR. - [x] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [x] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [x] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [x] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers)
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
Sep 24, 2024
## Summary This is a follow up to EUI's Emotion conversion of **EuiFormControlLayout/Delimited** (see elastic#190752, elastic/eui#7954, and elastic/eui#7957). > [!note] > Please manually QA your team's affected form control(s) to confirm they still look and behave as expected and are non-broken. The EUI team is not familiar enough with each plugin's setups to pull down and QA this PR ourselves. While QA testing the upgrade, I noticed a few incorrect usages of **EuiFormControlLayout** but wanted to wait until after the upgrade to push out fixes (to prevent delaying the PR further). In general, here is EUI's [recommended usage of the component](https://eui.elastic.co/#/forms/form-controls#form-control-layout): - Where possible, **simply don't use it**. Almost all form controls are **already** automatically wrapped in any EuiFormControlLayout by default, and should accept a large majority of the props that the layout accepts. - If you **must** use it, set the `controlOnly` prop on the child input/control to avoid buggy styling (e.g. duplicate borders). - If you can't do either of the above for any reason (e.g. missing prop support), reach out to the EUI team to ask for your UX as a feature request! ### Checklist Delete any items that are not applicable to this PR. - [x] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [x] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [x] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [x] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) (cherry picked from commit fd7b86e)
kibanamachine
added a commit
to elastic/kibana
that referenced
this pull request
Sep 24, 2024
# Backport This will backport the following commits from `main` to `8.x`: - [Fix various EuiFormControlLayout usages (#192779)](#192779) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Cee Chen","email":"549407+cee-chen@users.noreply.github.com"},"sourceCommit":{"committedDate":"2024-09-24T20:55:59Z","message":"Fix various EuiFormControlLayout usages (#192779)\n\n## Summary\r\n\r\nThis is a follow up to EUI's Emotion conversion of\r\n**EuiFormControlLayout/Delimited** (see\r\nhttps://github.com//pull/190752,\r\nhttps://github.com/elastic/eui/pull/7954, and\r\nhttps://github.com/elastic/eui/pull/7957).\r\n\r\n> [!note]\r\n> Please manually QA your team's affected form control(s) to confirm\r\nthey still look and behave as expected and are non-broken. The EUI team\r\nis not familiar enough with each plugin's setups to pull down and QA\r\nthis PR ourselves.\r\n\r\nWhile QA testing the upgrade, I noticed a few incorrect usages of\r\n**EuiFormControlLayout** but wanted to wait until after the upgrade to\r\npush out fixes (to prevent delaying the PR further). In general, here is\r\nEUI's [recommended usage of the\r\ncomponent](https://eui.elastic.co/#/forms/form-controls#form-control-layout):\r\n\r\n- Where possible, **simply don't use it**. Almost all form controls are\r\n**already** automatically wrapped in any EuiFormControlLayout by\r\ndefault, and should accept a large majority of the props that the layout\r\naccepts.\r\n- If you **must** use it, set the `controlOnly` prop on the child\r\ninput/control to avoid buggy styling (e.g. duplicate borders).\r\n- If you can't do either of the above for any reason (e.g. missing prop\r\nsupport), reach out to the EUI team to ask for your UX as a feature\r\nrequest!\r\n\r\n### Checklist\r\n\r\nDelete any items that are not applicable to this PR.\r\n\r\n- [x] Any UI touched in this PR is usable by keyboard only (learn more\r\nabout [keyboard accessibility](https://webaim.org/techniques/keyboard/))\r\n- [x] Any UI touched in this PR does not create any new axe failures\r\n(run axe in browser:\r\n[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),\r\n[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))\r\n- [x] This renders correctly on smaller devices using a responsive\r\nlayout. (You can test this [in your\r\nbrowser](https://www.browserstack.com/guide/responsive-testing-on-local-server))\r\n- [x] This was checked for [cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)","sha":"fd7b86e209e7133f3d9b7bd4e9fd6542f8a3aaad","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","EUI","v9.0.0","backport:prev-minor","ci:project-deploy-observability","Team:obs-ux-infra_services","Team:obs-ux-management","apm:review","v8.16.0"],"title":"Fix various EuiFormControlLayout usages","number":192779,"url":"https://github.com/elastic/kibana/pull/192779","mergeCommit":{"message":"Fix various EuiFormControlLayout usages (#192779)\n\n## Summary\r\n\r\nThis is a follow up to EUI's Emotion conversion of\r\n**EuiFormControlLayout/Delimited** (see\r\nhttps://github.com//pull/190752,\r\nhttps://github.com/elastic/eui/pull/7954, and\r\nhttps://github.com/elastic/eui/pull/7957).\r\n\r\n> [!note]\r\n> Please manually QA your team's affected form control(s) to confirm\r\nthey still look and behave as expected and are non-broken. The EUI team\r\nis not familiar enough with each plugin's setups to pull down and QA\r\nthis PR ourselves.\r\n\r\nWhile QA testing the upgrade, I noticed a few incorrect usages of\r\n**EuiFormControlLayout** but wanted to wait until after the upgrade to\r\npush out fixes (to prevent delaying the PR further). In general, here is\r\nEUI's [recommended usage of the\r\ncomponent](https://eui.elastic.co/#/forms/form-controls#form-control-layout):\r\n\r\n- Where possible, **simply don't use it**. Almost all form controls are\r\n**already** automatically wrapped in any EuiFormControlLayout by\r\ndefault, and should accept a large majority of the props that the layout\r\naccepts.\r\n- If you **must** use it, set the `controlOnly` prop on the child\r\ninput/control to avoid buggy styling (e.g. duplicate borders).\r\n- If you can't do either of the above for any reason (e.g. missing prop\r\nsupport), reach out to the EUI team to ask for your UX as a feature\r\nrequest!\r\n\r\n### Checklist\r\n\r\nDelete any items that are not applicable to this PR.\r\n\r\n- [x] Any UI touched in this PR is usable by keyboard only (learn more\r\nabout [keyboard accessibility](https://webaim.org/techniques/keyboard/))\r\n- [x] Any UI touched in this PR does not create any new axe failures\r\n(run axe in browser:\r\n[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),\r\n[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))\r\n- [x] This renders correctly on smaller devices using a responsive\r\nlayout. (You can test this [in your\r\nbrowser](https://www.browserstack.com/guide/responsive-testing-on-local-server))\r\n- [x] This was checked for [cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)","sha":"fd7b86e209e7133f3d9b7bd4e9fd6542f8a3aaad"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/192779","number":192779,"mergeCommit":{"message":"Fix various EuiFormControlLayout usages (#192779)\n\n## Summary\r\n\r\nThis is a follow up to EUI's Emotion conversion of\r\n**EuiFormControlLayout/Delimited** (see\r\nhttps://github.com//pull/190752,\r\nhttps://github.com/elastic/eui/pull/7954, and\r\nhttps://github.com/elastic/eui/pull/7957).\r\n\r\n> [!note]\r\n> Please manually QA your team's affected form control(s) to confirm\r\nthey still look and behave as expected and are non-broken. The EUI team\r\nis not familiar enough with each plugin's setups to pull down and QA\r\nthis PR ourselves.\r\n\r\nWhile QA testing the upgrade, I noticed a few incorrect usages of\r\n**EuiFormControlLayout** but wanted to wait until after the upgrade to\r\npush out fixes (to prevent delaying the PR further). In general, here is\r\nEUI's [recommended usage of the\r\ncomponent](https://eui.elastic.co/#/forms/form-controls#form-control-layout):\r\n\r\n- Where possible, **simply don't use it**. Almost all form controls are\r\n**already** automatically wrapped in any EuiFormControlLayout by\r\ndefault, and should accept a large majority of the props that the layout\r\naccepts.\r\n- If you **must** use it, set the `controlOnly` prop on the child\r\ninput/control to avoid buggy styling (e.g. duplicate borders).\r\n- If you can't do either of the above for any reason (e.g. missing prop\r\nsupport), reach out to the EUI team to ask for your UX as a feature\r\nrequest!\r\n\r\n### Checklist\r\n\r\nDelete any items that are not applicable to this PR.\r\n\r\n- [x] Any UI touched in this PR is usable by keyboard only (learn more\r\nabout [keyboard accessibility](https://webaim.org/techniques/keyboard/))\r\n- [x] Any UI touched in this PR does not create any new axe failures\r\n(run axe in browser:\r\n[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),\r\n[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))\r\n- [x] This renders correctly on smaller devices using a responsive\r\nlayout. (You can test this [in your\r\nbrowser](https://www.browserstack.com/guide/responsive-testing-on-local-server))\r\n- [x] This was checked for [cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)","sha":"fd7b86e209e7133f3d9b7bd4e9fd6542f8a3aaad"}},{"branch":"8.x","label":"v8.16.0","branchLabelMappingKey":"^v8.16.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT--> Co-authored-by: Cee Chen <549407+cee-chen@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Follow up to #7954
As always, please follow along by commit - this PR also contains a few necessary enhancements/fixes to
EuiFormControlLayoutandcloneElementWithCss.It also contains several updates/fixes to
EuiDualRange,EuiDatePickerRangeandEuiSuperDatePicker, as those 3 components are the main dogfooders of EuiFormControlLayoutDelimited.QA
Confirm the below links look the same as production:
General checklist
- [ ] Checked for accessibility including keyboard-only and screenreader modes- [ ] Added or updated jest and cypress tests- [ ] If applicable, added the breaking change issue label (and filled out the breaking change checklist)Emotion checklist
General
className(s)read as expected in snapshots and browsers[ ] Checked component playgroundUnit tests
shouldRenderCustomStyles()test was added and passes with parent component and any nestedchildProps(e.g.tooltipProps)[ ] Converted Enzyme to RTLSass/Emotion conversion process
src/components/index.scsssrc/amsterdam/overrides/{component}.scssfiles (styles within should have been converted to the baseline Emotion styles)$euiSizetoeuiTheme.size.base)[ ] Listed var/mixin removals in changelog[ ] Added an@warndeprecation message within theglobal_styling/mixins/{component}.scssfile[ ] Removed or converted component-specific Sass vars/mixins to exported JS versions[ ] Ranyarn compile-scssto update var/mixin JSON files[ ] Simplifiedcalc()tomathWithUnitsif possible (if mixing different unit types, this may not be possible)CSS tech debt
[ ] Wrapped all animations or transitions ineuiCanAnimate[ ] Usedgapproperty to add margin between items if using flex-inlineand-blocklogical properties (check inline styles as well as CSS)DOM Cleanup
euiComponent,euiComponent__child)Kibana due diligence
{euiComponent}-(case sensitive) to check for usage of modifier classes[ ] If usage exists, consider converting to adataattribute so that consumers still have something to hook into**/target, **/*.snap, **/*.storyshotfor less noise) foreui{Component}(case sensitive) to find:[ ] Any test/query selectors that will need to be updated[ ] Any Sass or CSS that will need to be updated, particularly if a component Sass var was deleted[ ] Any direct className usages that will need to be refactored (e.g. someone calling theNone, thankfullyeuiBadgeclass on a div instead of simply using theEuiBadgecomponent)Extras/nice-to-have
[ ] Optional component/code cleanup: consider splitting up the component into multiple children if it's overly verbose or difficult to reason about[ ] Documentation pass[ ] Check for issues in the backlog that could be a quick fix for that componentNo major Kibana customization/className usages 🤞