Skip to content
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

[Emotion] Convert EuiFormLayout (1/3) #7954

Merged
merged 18 commits into from
Aug 12, 2024

Conversation

cee-chen
Copy link
Contributor

@cee-chen cee-chen commented Aug 9, 2024

Summary

The large # of line diffs in this PR are mostly from snapshot changes - while this PR is on the larger size, I promise it's not as intimidating as it looks at first glance 🫠 As always, please follow along by commit!

Not handled in this PR (will be in future/separate PRs):

  • EuiFormControlLayoutDelimited
  • EuiFormControlLayoutIcons and EuiFormControlLayoutClearButton

QA

The below links should generally look non-broken/the same as production, except for a few key changes (listed in a comment below)

General checklist

  • Browser QA
    • Checked in both light and dark modes
    • Checked in mobile
    • Checked in Chrome, Safari, Edge, and Firefox
    • Checked for accessibility including keyboard-only and screenreader modes
  • Docs site QA - N/A
  • Code quality checklist
  • 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
  • Deleted any src/amsterdam/overrides/{component}.scss files (styles within should have been converted to the baseline Emotion styles)
  • Converted all global Sass vars/mixins to JS (e.g. $euiSize to euiTheme.size.base)
  • [ ] 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
  • [ ] Simplified calc() to mathWithUnits if possible (if mixing different unit types, this may not be possible)

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)
  • 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) None, thankfully

Extras/nice-to-have

  • Reduced specificity where possible (usually by reducing nesting and class name chaining)
  • [ ] 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

- entirely replaced by new `getIconAffordanceStyles` util, and CSS variables
+ remove unused euiButtonEmpty styles?? literally not sure what this is doing, button empty does not have borders. probably left over from old theme

+ reorder imports
- prefer using variables over mixins which contain unnecessary CSS
@cee-chen cee-chen force-pushed the emotion/form-layout branch from 3918432 to 949dfcf Compare August 9, 2024 20:07
Copy link
Contributor Author

@cee-chen cee-chen Aug 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intentional visual changes made as part of this PR:

  • EuiFormControlLayout - group border color was lightened to match non-grouped borders.
    • Before (slight jump when a prepend label is added):
      before
    • After:
      after
  • Slight height tweaks were made - buttons, text, and inputs should now all be more vertically centered
  • Slight horizontal spacing tweaks were made to arrays of multiple append/prepend nodes (with the goal of simplifying our CSS selectors)
    • It's a pretty low use-case scenario so I'm not too worried about it, but am open to feedback as to how it feels in comparison to prod (or generally)

package.json Outdated Show resolved Hide resolved
Comment on lines -101 to +109
'euiFormControlLayout--fullWidth': fullWidth,
'euiFormControlLayout--compressed': compressed,
'euiFormControlLayout--readOnly': readOnly,
'euiFormControlLayout--group': prepend || append,
'euiFormControlLayout--group': isGroup,
'euiFormControlLayout-isDisabled': isDisabled,
'euiFormControlLayout-readOnly': readOnly,
Copy link
Contributor Author

@cee-chen cee-chen Aug 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to clarify the modifier removals:

  • .euiFormControlLayout--group is used for various overrides in Kibana so I left it as-is
  • -readOnly is used for test selectors and felt like a state worth noting/preserving, so I left it but moved it down to one hyphen to match isDisabled

@cee-chen cee-chen force-pushed the emotion/form-layout branch from 949dfcf to f69e829 Compare August 9, 2024 21:02
Comment on lines -194 to +220
<>
{React.Children.map(nodes, (node, index) =>
typeof node === 'string'
? renderFormLabel(node)
: cloneElement(node, {
className: classNames(className, node.props.className),
key: index,
})
<div css={cssStyles} className={className}>
Copy link
Contributor Author

@cee-chen cee-chen Aug 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also wanted to quickly highlight this change: I opted to use a new <div> wrapper for append/prepend nodes as opposed to using React.cloneElement. It results it more snapshot changes, but I see this as a better best practice/pattern overall than cloning.

@cee-chen cee-chen marked this pull request as ready for review August 9, 2024 21:26
@cee-chen cee-chen requested a review from a team as a code owner August 9, 2024 21:26
@tkajtoch tkajtoch self-requested a review August 9, 2024 21:29
const { euiTheme } = euiThemeContext;
const form = euiFormVariables(euiThemeContext);

return `
/* We use inset box-shadow instead of border to skip extra hight calculations */
// We use inset box-shadow instead of border to skip extra height calculations
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Not a change request] To keep things simple, we might need to change these to regular borders when working on the visual refresh. I'm not entirely sure if using box-shadow in the past actually helped here enough to use it instead of a regular border 🤔 I'd love to hear more context if you know why exactly this is a thing!

Copy link
Contributor Author

@cee-chen cee-chen Aug 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would result in some amount of height recalculation shenanigans if we convert the style, but it shouldn't be too onerous.

I think the biggest change is that the underline we use for focus and invalid highlighting will look slightly different with a box-shadow vs a border (because right now the linear gradient overlaps the box-shadow, but will not overlap a border), but honestly I don't think that's a big enough reason to justify not using a border.

Overall I'd put it at a medium level of effort to change.

@@ -7,9 +7,9 @@
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking an extra step here and converting tests to RTL!

Copy link
Member

@tkajtoch tkajtoch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed commit by commit; the changes look and work great! I love the tiny improvements you added to make the form components look more polished 🎉

- most of the underlying inputs should already be setting their own styles without this component needing to (tested on all components)

- the background color also doesn't need to be set, that's specific to group styles anyway

- I removed the input border color change on readOnly as it doesn't match the default inputs - the border/box-shadow color stays the same for those, so I wanted that here as well
- border/background

+ extend `euiFormControlDefaultShadow` to allow passing only the box-shadow border and not the background gradient
- remove need for compressed/uncompressed CSS by just using `inherit`

- prefer setting a border radius + overflow hidden on the `childrenWrapper` instead of trying to set border radiuses on every possible child
@cee-chen cee-chen force-pushed the emotion/form-layout branch from f69e829 to af5978b Compare August 9, 2024 23:06
- [opinionated change] Add a `<div>` append/prepend wrapper instead of using `React.cloneElement` - less fragile and a better pattern

- use `:not:has` CSS to set padding on the wrapper instead of directly on child elements

- set heights on buttons and text more explicitly

- update form variables to be more specific and remove the unused border var
+ remove extra unnecessary border-radius logic in EuiComboBox - now already handled by EuiFormControlLayout
- works correctly in browser but causes test failures

- `:has()` support is apparently flaky, so we need to pin/revert the resolution
- several props (the ones destructured and re-spread to `<EuiFieldText />`) were not correctly updating the actual control

- note: this was already broken in prod
@cee-chen cee-chen force-pushed the emotion/form-layout branch 2 times, most recently from f601c97 to ea5830a Compare August 12, 2024 17:30
@cee-chen cee-chen force-pushed the emotion/form-layout branch from ea5830a to 8611ac8 Compare August 12, 2024 17:31
@cee-chen cee-chen enabled auto-merge (squash) August 12, 2024 17:37
@cee-chen cee-chen disabled auto-merge August 12, 2024 17:37
@cee-chen cee-chen enabled auto-merge (squash) August 12, 2024 17:37
@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

@cee-chen cee-chen merged commit d4dc2f6 into elastic:main Aug 12, 2024
5 checks passed
@cee-chen cee-chen deleted the emotion/form-layout branch August 12, 2024 18:00
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>
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
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants