Fix various EuiFormControlLayout usages#192779
Merged
Ikuni17 merged 18 commits intoelastic:mainfrom Sep 24, 2024
Merged
Conversation
…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
- 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
cee-chen
added
release_note:skip
6 tasks
Contributor
|
Pinging @elastic/eui-team (EUI) |
dominiqueclarke
approved these changes
Sep 17, 2024
Contributor
dominiqueclarke
left a comment
dominiqueclarke
left a comment
There was a problem hiding this comment.
Obs ux management changes LGTM
szwarckonrad
requested changes
Sep 17, 2024
Contributor
szwarckonrad
left a comment
szwarckonrad
left a comment
There was a problem hiding this comment.
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
approved these changes
Sep 17, 2024
Contributor
jeramysoucy
left a comment
jeramysoucy
left a comment
There was a problem hiding this comment.
Kibana security changes LGTM!
Contributor
There was a problem hiding this comment.
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
reviewed
Sep 18, 2024
Contributor
rylnd
left a comment
rylnd
left a comment
There was a problem hiding this comment.
@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
Contributor
Author
|
@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
szwarckonrad
approved these changes
Sep 20, 2024
Contributor
szwarckonrad
left a comment
szwarckonrad
left a comment
There was a problem hiding this comment.
No regression spotted with the latest changes. Thank you for addressing these! 🙌
TattdCodeMonkey
approved these changes
Sep 20, 2024
- make the form element compressed to match the rest of the form controls in the panel - remove unnecessary `EuiFormLabel` - one gets auto generated if we just pass a string - move className override to parent and target the `__prepend` className
Contributor
Author
|
@elastic/kibana-visualizations Once CI passes, I'll be requesting an admin merge from Kibana Ops. This PR contains several fixes for other teams that I don't want to hold up further. I've pulled down and tested the UI owned by your team (Maps and Annotations) and confirm that they look and behave as expected: Annotations:
Maps:
|
💚 Build Succeeded
Metrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
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)
Contributor
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
kibanamachine
added a commit
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>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
16 participants
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
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:
controlOnlyprop on the child input/control to avoid buggy styling (e.g. duplicate borders).Checklist
Delete any items that are not applicable to this PR.