Fix various EuiFormControlLayout usages #192779

cee-chen · 2024-09-12T21:12:36Z

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:

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.

Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

…ly` props - prevents duplicate borders and other buggy effects

- EuiDatePicker supports `prepend` now, so move the prop directly onto the component - EuiDatePicker also supports being cleared via `onClear`, no need to add a control layout for that button

- the component already comes wrapped with EuiFormControlLayout by default, so we just need to move the props to the component itself

…s + fix empty label - should be using `hasEmptyLabelSpace` prop instead of a space - form controls already come wrapped with EuiFormControlLayout by default, so we just need to move the props to the component itself

…apper - field search components already support being cleared by default - we need to move the onClear logic to the callbacks instead

elasticmachine · 2024-09-13T00:03:59Z

Pinging @elastic/eui-team (EUI)

yngrdyn

LGTM

elasticmachine · 2024-09-13T08:05:50Z

Pinging @elastic/obs-ux-infra_services-team (Team:obs-ux-infra_services)

elasticmachine · 2024-09-13T08:05:50Z

Pinging @elastic/obs-ux-management-team (Team:obs-ux-management)

adcoelho

Tested the triggers_actions_ui changed and everything still works as expected 👍

Approved for response-ops

Heenawter

Book example embeddable + maps custom icon picker changes LGTM. Code review + tested locally to ensure that the HTML looks as expected - verified that the Maps EuiFieldText component is no longer wrapped in a double EuiFormControlLayout

cauemarcondes

LGTM

jeramysoucy · 2024-09-13T16:15:16Z

x-pack/packages/security/api_key_management/src/components/token_field.tsx

This does not look like it is working as expected. The copy icon button is no longer rendering and it's missing the drop-down.

This PR:

Main:

Let me know if this is working now with the latest commit! Thank you so much for catching this! 🙇

The format drop-down and copy button now appear. The copy button is working, but the format selector is a bit strange. When "Encoded" is selected, it is highlighted in the list, but when "Beats" is selected, "Logstash" is highlighted in the list. When "Logstash" is selected, nothing is highlighted in the list. The format of the key for Beats and Logstash are identical, not sure if that is correct.

I have confirmed the exact same behavior in Main, so I will approve. Not sure if this is an issue with our code or the EUI components. I'll open an issue to look more closely.

Issue: #193127

I poked into this a bit locally and was able to fix the issue! Thankfully just a small oneliner. 661273c

x-pack/packages/security/api_key_management/src/components/token_field.tsx

ryankeairns

SCSS check

dominiqueclarke

Obs ux management changes LGTM

szwarckonrad

DW changes introduce regressions in functionality.

main

Screen.Recording.2024-09-17.at.10.02.37.mov

cee-chen:eui-form-layout-fixes

Screen.Recording.2024-09-17.at.09.53.12.mov

jeramysoucy

Kibana security changes LGTM!

jeramysoucy · 2024-09-17T09:20:07Z

x-pack/packages/security/api_key_management/src/components/token_field.tsx

The format drop-down and copy button now appear. The copy button is working, but the format selector is a bit strange. When "Encoded" is selected, it is highlighted in the list, but when "Beats" is selected, "Logstash" is highlighted in the list. When "Logstash" is selected, nothing is highlighted in the list. The format of the key for Beats and Logstash are identical, not sure if that is correct.

I have confirmed the exact same behavior in Main, so I will approve. Not sure if this is an issue with our code or the EUI components. I'll open an issue to look more closely.

rylnd

@cee-chen the Detection Engine changes look to be equivalent to main for ScheduleItem, (which doesn't look correct to me, but I think that's a separate issue:

), I'm seeing the following differences in DurationInput :

Extra border where previously there was none:
Disabling the input leads to an even more drastic change relative to the container:
before:
after:

I'd be happy to pair if you need help here, please reach out on Slack.

- `searchValue` state is a step behind the search value string passed by EUI, so use that value instead to ensure we're not proceeding with `onSearch` split logic

- a lot of this can be cleaned up - `!important`s aren't ideal but are the fastest way forward here

cee-chen · 2024-09-18T19:22:29Z

@szwarckonrad @rylnd Thank you both for the thorough QA and help with repro steps! I should have fixed both behaviors, but please let me know if not!

Security defend workflows:

(ignore the lastpass autofill at the end, that's my browser/extension 😅)

Security detection engine:

`* 2` index is unnecessary, EuiContextMenuPanel accounts for non-interactive elements

rylnd

@cee-chen thank you so much for fixing those issues. The Detection Engine changes look great!

kibana-ci · 2024-09-18T20:46:14Z

💚 Build Succeeded

Buildkite Build
Commit: 661273c
Kibana Serverless Image: docker.elastic.co/kibana-ci/kibana-serverless:pr-192779-661273c3e548

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`apm`	3.5MB	3.5MB	+15.0B
`enterpriseSearch`	2.6MB	2.6MB	+15.0B
`eventAnnotationListing`	227.1KB	226.8KB	-317.0B
`infra`	1.6MB	1.6MB	-156.0B
`lens`	1.5MB	1.5MB	-317.0B
`maps`	3.0MB	3.0MB	-118.0B
`observability`	463.7KB	463.6KB	-116.0B
`profiling`	405.9KB	405.8KB	-110.0B
`security`	589.3KB	589.2KB	-105.0B
`securitySolution`	20.4MB	20.4MB	-911.0B
`serverlessSearch`	326.9KB	326.8KB	-105.0B
`synthetics`	964.1KB	964.1KB	+30.0B
`triggersActionsUi`	1.6MB	1.6MB	-74.0B
total			-2.2KB

History

💛 Build #234522 was flaky 399ac09
💛 Build #234119 was flaky 0bd64d7
💛 Build #233882 was flaky 50ef939
💔 Build #233867 failed 98def99

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

szwarckonrad

No regression spotted with the latest changes. Thank you for addressing these! 🙌

cee-chen added 7 commits September 12, 2024 14:08

[EuiFormControlLayout/Delimited] Fix nested inputs missing `controlOn…

a7e19bf

…ly` props - prevents duplicate borders and other buggy effects

[examples] remove incorrect usage of EuiFormControlLayout

bfbe13c

[EuiDatePicker] Remove unnecessary EuiFormControlLayout wrappers

7a3e9a0

- EuiDatePicker supports `prepend` now, so move the prop directly onto the component - EuiDatePicker also supports being cleared via `onClear`, no need to add a control layout for that button

[EuiFieldText] Remove unnecessary usages of EuiFormControlLayout

839f756

- the component already comes wrapped with EuiFormControlLayout by default, so we just need to move the props to the component itself

[EuiFieldNumber] Remove unnecessary usages of EuiFormControlLayout

417782f

- the component already comes wrapped with EuiFormControlLayout by default, so we just need to move the props to the component itself

[EuiFieldSearch] Remove unnecessary EuiFormControlLayout.onClear wr…

98def99

…apper - field search components already support being cleared by default - we need to move the onClear logic to the callbacks instead

cee-chen added release_note:skip Skip the PR/issue when compiling release notes EUI v8.16.0 labels Sep 12, 2024

cee-chen mentioned this pull request Sep 12, 2024

[EuiDatePicker] Fix clear button appearing when input is disabled elastic/eui#8020

Merged

6 tasks

Update snapshots

50ef939

cee-chen marked this pull request as ready for review September 13, 2024 00:03

cee-chen requested review from a team as code owners September 13, 2024 00:03

cee-chen requested review from joeypoon, szwarckonrad and rylnd September 13, 2024 00:03

smith added the apm:review label Sep 13, 2024

yngrdyn approved these changes Sep 13, 2024

View reviewed changes

botelastic bot added ci:project-deploy-observability Create an Observability project Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team Team:obs-ux-management Observability Management User Experience Team labels Sep 13, 2024

adcoelho approved these changes Sep 13, 2024

View reviewed changes

Heenawter self-requested a review September 13, 2024 14:01

Heenawter reviewed Sep 13, 2024

View reviewed changes

Heenawter approved these changes Sep 13, 2024

View reviewed changes

cauemarcondes approved these changes Sep 13, 2024

View reviewed changes

jeramysoucy requested changes Sep 13, 2024

View reviewed changes

cee-chen commented Sep 13, 2024

View reviewed changes

x-pack/packages/security/api_key_management/src/components/token_field.tsx Outdated Show resolved Hide resolved

Fix API token field missing append/prepend

0bd64d7

ryankeairns approved these changes Sep 16, 2024

View reviewed changes

Merge branch 'main' into eui-form-layout-fixes

399ac09

dominiqueclarke approved these changes Sep 17, 2024

View reviewed changes

szwarckonrad requested changes Sep 17, 2024

View reviewed changes

jeramysoucy approved these changes Sep 17, 2024

View reviewed changes

Merge branch 'main' into eui-form-layout-fixes

c46ddf2

rylnd reviewed Sep 18, 2024

View reviewed changes

cee-chen added 2 commits September 18, 2024 11:13

[PR feeedback] Fix defend workflows UX regression

def4600

- `searchValue` state is a step behind the search value string passed by EUI, so use that value instead to ensure we're not proceeding with `onSearch` split logic

[PR feedback] Fix security rule schedule append+select styling

b0418cd

- a lot of this can be cleaned up - `!important`s aren't ideal but are the fastest way forward here

[bonus] Fix incorrect selection state on API key format dropdown

661273c

`* 2` index is unnecessary, EuiContextMenuPanel accounts for non-interactive elements

rylnd approved these changes Sep 18, 2024

View reviewed changes

cee-chen requested a review from szwarckonrad September 19, 2024 16:24

szwarckonrad approved these changes Sep 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix various EuiFormControlLayout usages #192779

Fix various EuiFormControlLayout usages #192779

cee-chen commented Sep 12, 2024 •

edited

Loading

elasticmachine commented Sep 13, 2024

yngrdyn left a comment

elasticmachine commented Sep 13, 2024

elasticmachine commented Sep 13, 2024

adcoelho left a comment

Heenawter left a comment

cauemarcondes left a comment

jeramysoucy Sep 13, 2024 •

edited

Loading

cee-chen Sep 13, 2024 •

edited

Loading

jeramysoucy Sep 17, 2024

jeramysoucy Sep 17, 2024

cee-chen Sep 18, 2024

ryankeairns left a comment

dominiqueclarke left a comment

szwarckonrad left a comment

jeramysoucy left a comment

jeramysoucy Sep 17, 2024

rylnd left a comment

cee-chen commented Sep 18, 2024 •

edited

Loading

rylnd left a comment

kibana-ci commented Sep 18, 2024 •

edited

Loading

szwarckonrad left a comment

Fix various EuiFormControlLayout usages #192779

Are you sure you want to change the base?

Fix various EuiFormControlLayout usages #192779

Conversation

cee-chen commented Sep 12, 2024 • edited Loading

Summary

Checklist

elasticmachine commented Sep 13, 2024

yngrdyn left a comment

Choose a reason for hiding this comment

elasticmachine commented Sep 13, 2024

elasticmachine commented Sep 13, 2024

adcoelho left a comment

Choose a reason for hiding this comment

Heenawter left a comment

Choose a reason for hiding this comment

cauemarcondes left a comment

Choose a reason for hiding this comment

jeramysoucy Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

cee-chen Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

jeramysoucy Sep 17, 2024

Choose a reason for hiding this comment

jeramysoucy Sep 17, 2024

Choose a reason for hiding this comment

cee-chen Sep 18, 2024

Choose a reason for hiding this comment

ryankeairns left a comment

Choose a reason for hiding this comment

dominiqueclarke left a comment

Choose a reason for hiding this comment

szwarckonrad left a comment

Choose a reason for hiding this comment

jeramysoucy left a comment

Choose a reason for hiding this comment

jeramysoucy Sep 17, 2024

Choose a reason for hiding this comment

rylnd left a comment

Choose a reason for hiding this comment

cee-chen commented Sep 18, 2024 • edited Loading

rylnd left a comment

Choose a reason for hiding this comment

kibana-ci commented Sep 18, 2024 • edited Loading

💚 Build Succeeded

Metrics [docs]

Async chunks

History

szwarckonrad left a comment

Choose a reason for hiding this comment

cee-chen commented Sep 12, 2024 •

edited

Loading

jeramysoucy Sep 13, 2024 •

edited

Loading

cee-chen Sep 13, 2024 •

edited

Loading

cee-chen commented Sep 18, 2024 •

edited

Loading

kibana-ci commented Sep 18, 2024 •

edited

Loading