[Emotion] Convert EuiFormRow by cee-chen · Pull Request #7968 · elastic/eui

cee-chen · 2024-08-17T00:44:28Z

Summary

Part of #6400

This one is a doozy 🙃 EuiFormRow is a 7 year old component with a lot of outdated/not-best practices and undocumented props. I tried to clean up a few while here but there's so many usages in Kibana that I got terrified halfway through and just ended up settling for deprecations instead 🫠

As always, please review by commit!

Technical details around opinionated changes:

Converted EuiFormRow from a class to a function component, and removed onFocus/onBlur being cloned onto child inputs in favor of onFocus/onBlur on a parent div (which captures focus/blur via bubbled events)
Changed margins to flex gap CSS
Removed .euiFormRow--hasSwitch in favor of :has() CSS. Removed .euiFormRow--compressed .euiFormRow--horizontal modifiers as well
Marked display="columnCompressedSwitch" as deprecated as a result (it's the same as columnCompressed now). Also deprecated display="rowCompressed" as it doesn't appear to do anything 🤷
Tweaked hasEmptyLabelSpace prop - it now renders an <EuiSpacer /> instead of using margin-top CSS, in order to take advantage of the new flex gap CSS. Also, DRYed out its vertical centering css with display="center", which allows us to remove the .euiFormRow__fieldWrapperDisplayOnly className (no Kibana usages).

QA

https://eui.elastic.co/pr_7968/#/forms/compressed-forms should look the same as prod
https://eui.elastic.co/pr_7968/#/forms/form-layouts#inline should look the same as prod
https://eui.elastic.co/pr_7968/storybook/?path=/story/forms-euiform-euiformrow--column-layout
https://eui.elastic.co/pr_7968/storybook/?path=/story/forms-euiform-euiformrow--inline-layout

General checklist

Browser QA
- Checked in mobile
- Checked in Chrome, Safari, Edge, and Firefox
  ~~- [ ] Checked in both light and dark modes~~ - No colors, just spacing
  ~~- [ ] Checked for accessibility including keyboard-only and screenreader modes~~
Docs site QA
- Updated documentation
- Props have proper autodocs (using @default if default values are missing) and playground toggles
  ~~- [ ] Checked Code Sandbox works for any docs examples~~
Code quality checklist
- [ ] Added or updated jest and cypress tests
- Updated visual regression tests
Release checklist
- A changelog entry exists and is marked appropriately.
  ~~- [ ] If applicable, added the breaking change issue label (and filled out the breaking change checklist)~~
Designer checklist - N/A

Emotion checklist

General

Output CSS matches the previous CSS (works as expected in all browsers)
Rendered className(s) read as expected in snapshots and browsers
~~[ ] Checked component playground~~

Unit tests

shouldRenderCustomStyles() test was added and passes with parent component and any nested childProps (e.g. tooltipProps)
Converted Enzyme to RTL

Sass/Emotion conversion process

Removed component from src/components/index.scss
Converted all global Sass vars/mixins to JS (e.g. $euiSize to euiTheme.size.base)
~~[ ] Simplified calc() to mathWithUnits if possible (if mixing different unit types, this may not be possible)~~ - not possible
~~[ ] Listed var/mixin removals in changelog~~
~~[ ] Added an @warn deprecation message within the global_styling/mixins/{component}.scss file~~
~~[ ] Removed or converted component-specific Sass vars/mixins to exported JS versions~~
~~[ ] Ran yarn compile-scss to update var/mixin JSON files~~
~~[ ] Deleted any src/amsterdam/overrides/{component}.scss files (styles within should have been converted to the baseline Emotion styles)~~

CSS tech debt

~~[ ] Wrapped all animations or transitions in euiCanAnimate~~
Used gap property to add margin between items if using flex
Converted side specific padding, margin, and position to -inline and -block logical properties (check inline styles as well as CSS)

DOM Cleanup

~~[ ] Did NOT remove any block/element classNames (e.g. euiComponent, euiComponent__child)~~ - Deleted one with no Kibana usages
SEARCH KIBANA FIRST: Deleted any modifier classNames or maps if not being used in Kibana.

Kibana due diligence

Search Kibana's codebase for {euiComponent}- (case sensitive) to check for usage of modifier classes
- ~~[ ] If usage exists, consider converting to a data attribute so that consumers still have something to hook into~~
Pre-emptively check how your conversion will impact the next Kibana upgrade. This entails searching/grepping through Kibana (excluding **/target, **/*.snap, **/*.storyshot for less noise) for eui{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 the euiBadge class on a div instead of simply using the EuiBadge component)

Extras/nice-to-have

~~[ ] Reduced specificity where possible (usually by reducing nesting and class name chaining)~~ Skipped, not worth it for this component IMO - already a headache to deal with
~~[ ] 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 component~~

+ update unnecessary onFocus/onBlur child behavior - these events already bubble in React, so we just need to set them on a wrapper instead of cloning the children directly

…count for focus changes

+ convert margins to flex gap instead

- switch width to flex-basis so we don't need to use logical property - simplify line-height, it's more centered without the 1px - remove unnecessary display block unset and margin-bottom (nothing to unset)

- honestly not sure why this prop is even here but oh well! - use fancy JS getters since we're no longer setting `row` as the default CSS

- use CSS `:has` instead of a separate display prop (deprecate eventually) - simplify margin logic, I found the existing CSS harder to follow than - remove unnecessary duplicated CSS (width/min-width), simplify line-height

…ay props as deprecated + remove them from docs examples

- replace margin with an EuiSpacer instead, so we can take advantage of the new gap CSS instead of having to add up to get 20px - use some trickery to duplicate existing `center` CSS (since this is essentially what the min-height/justify-content does - except doing it this way will allow it to work for column layouts as well) - remove padding-bottom, not doing anything - delete `.euiFormRow__fieldWrapperDisplayOnly` className while here, no usages in Kibana

JasonStoltz

Hmmm, is this a bug with hasEmptyLabelSpace?

Not sure exactly what's causing it

tkajtoch · 2024-08-19T18:47:26Z

packages/eui/src/components/form/form_row/form_row.styles.ts

+        )}
+      }
+    `,
+    get center() {


[Not a change request, just trying to understand the exact reason :D] Are these two getters so you can access this.row and this.centerDisplayCss? I'd normally prefer to move centerDisplayCss above this so we could get rid of these getters but since row does the same thing it feels clean to do it like this here

Yes, I use getters are for this access/repeating styles: https://github.com/elastic/eui/blob/main/wiki/contributing-to-eui/developing/writing-styles-with-emotion.md#duplicate-styles

And yes, it's essentially repeating row styles hence me trying to write it a bit more DRYly. It's not perfect but to be honest the center display is a little weird and I'd rather get rid of it entirely if we can in the future 🤷

tkajtoch · 2024-08-19T18:51:17Z

Code changes look great! Thanks for deprecating the compressed props and updating docs not to use them. I'll leave the QA to Jason since he's already reviewing the PR

JasonStoltz

I did quite a bit of QA. Haven't been able to break it other than the label issue I posted.

cee-chen · 2024-08-19T19:31:43Z

Hmmm, is this a bug with hasEmptyLabelSpace?

hasEmptyLabelSpace is primarily meant to be used with inline rows and centered content (see https://eui.elastic.co/pr_7968/storybook/?path=/story/forms-euiform-euiformrow--inline-layout) so I updated its styles to match accordingly. To be honest I don't love it as a prop and I'd rather deprecate it + towards another component that handles inline rows instead, but that was way out of scope for this PR.

I can try to update the storybook control to be less confusing here in a bit, but essentially that prop isn't really meant for that type of example.

…empty

JasonStoltz · 2024-08-19T20:23:55Z

@cee-chen Ok. I'm just worried because the behavior is different before and after the changes:

Before:

After:

I see the hasEmptyLabelSpace prop used 24 times and none of the usages are on form rows with display=center.

This one looks like the exact case I'm testing -- https://github.com/elastic/kibana/blob/main/x-pack/plugins/snapshot_restore/public/application/components/restore_snapshot_form/steps/step_logistics/step_logistics.tsx#L603 -- display is row, help text is present, and has hasEmptyLabelSpace is true. I'm just assuming (without testing) in Kibana that that would break.

JasonStoltz

I'm approving. We'll just need to check Kibana when it gets upgraded.

cee-chen · 2024-08-19T20:57:16Z

Looking at step_logistic specifically in prod, the hasEmptyLabelSpace prop isn't actually even doing anything for them when used within a EuiDescribedForm - so we should get rid of it completely

EDIT: I went through every hasEmptyLabelSpace usage in Kibana and have reversed trains of thought - people are using it with all kinds of column displays and children 💀 I'm going to go ahead and removed the flex centering added to it and made the changelog clearer about what it does and what alternatives people have if their UI is less centered than they expect.

- wasn't introduced in this PR but in the previous EuiFormLayout one - might as well fix it while we're here

elasticmachine · 2024-08-19T22:17:05Z

💚 Build Succeeded

Buildkite Build
Commit: 82c1ca8

History

💚 Build #2613 succeeded f2bb478
💚 Build #2597 succeeded b0cb928
💔 Build #2595 failed 19cf97a

cc @JasonStoltz

`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>

…Switch"` prop (#198709) ## Summary This prop option has been deprecated as of elastic/eui#7968. `display="columnCompressed"` now automatically accounts for EuiSwitches (via `:has` CSS) and the extra prop option is no longer needed. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [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)) --------- Co-authored-by: Marta Bondyra <4283304+mbondyra@users.noreply.github.com>

…Switch"` prop (elastic#198709) ## Summary This prop option has been deprecated as of elastic/eui#7968. `display="columnCompressed"` now automatically accounts for EuiSwitches (via `:has` CSS) and the extra prop option is no longer needed. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [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)) --------- Co-authored-by: Marta Bondyra <4283304+mbondyra@users.noreply.github.com> (cherry picked from commit 83a9acd)

…;display="columnCompressedSwitch"` prop (#198709) (#199210) # Backport This will backport the following commits from `main` to `8.x`: - [Remove deprecated usages of `EuiFormRow`'s `display="columnCompressedSwitch"` prop (#198709)](#198709)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Cee Chen <549407+cee-chen@users.noreply.github.com>

cee-chen added 2 commits August 15, 2024 13:20

[tech debt] Convert EuiFormRow from a class to a function component

296d1c3

+ update unnecessary onFocus/onBlur child behavior - these events already bubble in React, so we just need to set them on a wrapper instead of cloning the children directly

[tests tech debt] Convert Enzyme to RTL + update behavior tests to ac…

9323d7f

…count for focus changes

cee-chen added the emotion label Aug 17, 2024

cee-chen added 5 commits August 16, 2024 17:46

Convert basic styles

6e20d9a

Convert basic differentiated styles between row and column layout

a91903a

+ convert margins to flex gap instead

Convert remaining row vs column styles

d726757

- switch width to flex-basis so we don't need to use logical property - simplify line-height, it's more centered without the 1px - remove unnecessary display block unset and margin-bottom (nothing to unset)

Convert center display styling

be845c5

- honestly not sure why this prop is even here but oh well! - use fancy JS getters since we're no longer setting `row` as the default CSS

Convert column switch-specific CSS

019b3d1

- use CSS `:has` instead of a separate display prop (deprecate eventually) - simplify margin logic, I found the existing CSS harder to follow than - remove unnecessary duplicated CSS (width/min-width), simplify line-height

cee-chen force-pushed the emotion/form-row branch from 19cf97a to d802d03 Compare August 17, 2024 05:33

cee-chen added 5 commits August 16, 2024 22:45

Update docs to mark rowCompressed and columCompressedSwitch displ…

9f95c6e

…ay props as deprecated + remove them from docs examples

Delete Sass files

f8b64fa

[stories] Add VRT/examples for column and inline form row layouts

dfa9f10

Update downstream snapshots

b0cb928

cee-chen force-pushed the emotion/form-row branch from d802d03 to b0cb928 Compare August 17, 2024 05:45

cee-chen marked this pull request as ready for review August 17, 2024 06:18

cee-chen requested a review from a team as a code owner August 17, 2024 06:18

JasonStoltz self-assigned this Aug 19, 2024

cee-chen mentioned this pull request Aug 19, 2024

[Emotion] Convert EuiCheckbox, EuiRadio, and EuiSwitch #7969

Merged

8 tasks

JasonStoltz requested changes Aug 19, 2024

View reviewed changes

tkajtoch reviewed Aug 19, 2024

View reviewed changes

JasonStoltz reviewed Aug 19, 2024

View reviewed changes

[storybook] Only show hasEmptyLabelSpace control if label field is …

f2bb478

…empty

JasonStoltz approved these changes Aug 19, 2024

View reviewed changes

cee-chen added 2 commits August 19, 2024 14:38

[PR feedback] Remove centering styling from hasEmptyLabelSpace

58f3255

[misc] fix prepend/append bug

82c1ca8

- wasn't introduced in this PR but in the previous EuiFormLayout one - might as well fix it while we're here

cee-chen force-pushed the emotion/form-row branch from da050fc to 82c1ca8 Compare August 19, 2024 21:50

cee-chen enabled auto-merge (squash) August 19, 2024 21:50

cee-chen merged commit ef56556 into elastic:main Aug 19, 2024

cee-chen deleted the emotion/form-row branch August 19, 2024 22:17

cee-chen mentioned this pull request Aug 20, 2024

Upgrade EUI to v95.9.0 elastic/kibana#190752

Merged

cee-chen mentioned this pull request Aug 29, 2024

[Meta] Deprecation and Beta schedule #1469

Open

39 tasks

cee-chen mentioned this pull request Nov 1, 2024

Remove deprecated usages of EuiFormRow's display="columnCompressedSwitch" prop elastic/kibana#198709

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

[Emotion] Convert EuiFormRow#7968

[Emotion] Convert EuiFormRow#7968
cee-chen merged 15 commits intoelastic:mainfrom
cee-chen:emotion/form-row

cee-chen commented Aug 17, 2024 •

edited

Loading

Uh oh!

JasonStoltz left a comment

Uh oh!

tkajtoch Aug 19, 2024 •

edited

Loading

Uh oh!

cee-chen Aug 19, 2024

Uh oh!

tkajtoch commented Aug 19, 2024

Uh oh!

JasonStoltz left a comment

Uh oh!

cee-chen commented Aug 19, 2024 •

edited

Loading

Uh oh!

JasonStoltz commented Aug 19, 2024 •

edited

Loading

Uh oh!

JasonStoltz left a comment

Uh oh!

cee-chen commented Aug 19, 2024 •

edited

Loading

Uh oh!

elasticmachine commented Aug 19, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Comments

Conversation

cee-chen commented Aug 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

QA

General checklist

Uh oh!

JasonStoltz left a comment

Choose a reason for hiding this comment

Uh oh!

tkajtoch Aug 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cee-chen Aug 19, 2024

Choose a reason for hiding this comment

Uh oh!

tkajtoch commented Aug 19, 2024

Uh oh!

JasonStoltz left a comment

Choose a reason for hiding this comment

Uh oh!

cee-chen commented Aug 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JasonStoltz commented Aug 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JasonStoltz left a comment

Choose a reason for hiding this comment

Uh oh!

cee-chen commented Aug 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Aug 19, 2024

💚 Build Succeeded

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cee-chen commented Aug 17, 2024 •

edited

Loading

tkajtoch Aug 19, 2024 •

edited

Loading

cee-chen commented Aug 19, 2024 •

edited

Loading

JasonStoltz commented Aug 19, 2024 •

edited

Loading

cee-chen commented Aug 19, 2024 •

edited

Loading