[EuiDataGrid] Fix inconsistent focus halo#5522
Conversation
The outline property for the control columns of the EuiDataGrid element is modified in order to keep color theme consistent along the grid.
The outline of euiButtonIcons is moved to the _data_grid_data_row in order to not modify outlines in buttons outside of euiDataGrids
|
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
|
💚 CLA has been signed |
Added a more complete description of the changes made.
Al-0
changed the title
|
jenkins test this |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5522/ |
|
@thompsongl sorry, I'm kinda new to this, how do I Jenkin test my PR? |
No worries! A maintainer needs to start the CI process for you. |
elizabetdev
left a comment
elizabetdev
left a comment
There was a problem hiding this comment.
Thanks, @Al-0 for opening this PR,
It's looking good! 🎉
But we don't want to change the styles in euiCustomControlFocused() mixin in __form.scss. This changes multiple components and we just want to enhance the focus states for the EuiDataGrid.
So I would recommend overriding the control column checkboxes focus styles rather than updating all EUI checkboxes focus states.
|
Thanks for the review @miukimiu ! |
The following changes are made: * CHANGELOG.md: Title is changed to enhancements and description is updated. * _form.scss: File is restored to its original state, as it was modyfing a whole mixin. * _dara_grid_data/header_row.scss: The property from the mixin is moved to each of these files to overwrite the checkbox modifiers.
…nt-halo-euiDataGrid
`$euiFocusRingColor`
|
jenkins test this |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5522/ |
elizabetdev
left a comment
elizabetdev
left a comment
There was a problem hiding this comment.
Thanks @Al-0, 🎉
Your PR looks great but internally our team decided that we shouldn't have specific outlines styles only for some components. With time, this would bring inconsistency. For example, in chrome, the data grid checkboxes would have blue outlines, and then in other components, they would be black.
However, we agree that the black outline creates some confusion for our consumers so our team decided that we're going to update them so that they always be blue no matter which browser. We see this as an enhancement and not a bug fix. So we are not in a rush, and we prefer to update all focus rings/outlines in different PRs.
Because the original issue #5479 was related to checkboxes I pushed a commit where I'm updating the mixing euiCustomControlFocused to have an outline offset of 2px which makes it consistent with our range slider.
From now on, checkboxes and radio have a blue outline no matter the browser.
For buttons outlines, we're keeping the current color. We prefer to tackle this in another PR. We have to think if we want to have the blue outline for all the buttons or keep the current color (warning, danger...) and possibly just update the black outlines.
Let's wait for @1Copenut approval and if everything seems ok, we can merge the PR! 😄
|
Very well @miukimiu, I understand the reason behind the change and greatly appreciate the explanation provided. Thank you very much! It has been very interesting and a great experience working on this PR! |
1Copenut
left a comment
1Copenut
left a comment
There was a problem hiding this comment.
🎉 This looks good to me as well. I pulled the branch to my local machine and tested with Chrome on MacOS Big Sur. The change helps me distinguish checkbox focus in data grids and standalone checkboxes and radio buttons.
Thank you all for the conversation and enhancement!
…nt-halo-euiDataGrid
|
jenkins test this |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5522/ |
5 participants
Summary
Closes #5479
The outline property for the control columns of the EuiDataGrid element is modified in order to keep color theme consistent along the grid. The
outlineproperty ofeuiCustomControlFocusedis fixed, as it failed due to not being properly declared for chrome and being declared in the incorrect order for other browsers. Also added the property for_data_grid_data_rowfor entries with class.euiButtonIcon, as also the columns with this class didn't comply with the grid's color.Checklist
- [ ] Props have proper autodocs and playground toggles- [ ] Added or updated jest and cypress tests