Order Emotion styles before Sass styles #161592
Merged
Conversation
This file contains 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
- We're at the point now where almost all low-level EUI atoms are in Emotion, and any remaining EUI Sass are larger molecules that should override EUI atoms
|
Pinging @elastic/eui-team (EUI) |
cee-chen
added
the
release_note:skip
|
@elastic/platform-design and @elastic/eui-team - is there any chance I could tag you all in for help with QAing / smoke-testing as much of Kibana as possible for this change? I'm a little worried about unintended side effects as a result of this cascading specificity change. What I've glanced at so far (mostly overview pages) still look like they're working for me, but I'm sure there's some unpredictable hidden edge case or visual regressions out there as a result of some custom styles. |
cee-chen
changed the title
jbudz
approved these changes
Jul 10, 2023
|
@elasticmachine merge upstream |
pgayvallet
approved these changes
Jul 11, 2023
- Since EUI Sass now overrides Emotion :(
|
@elasticmachine merge upstream |
- remove unused `css` prop on column - fix chart not appearing due to unset width
|
@elastic/kibana-data-discovery and @elastic/ml-ui - we found some visual regressions as a result of ordering Sass styles after Emotion styles. We'd super appreciate y'all doing a quick visual scan / smoke test of your app to ensure nothing else regressed.
|
davismcphee
approved these changes
Jul 12, 2023
| @@ -116,7 +116,6 @@ export const ChangePointsTable: FC<ChangePointsTableProps> = ({ | |||
| height: '80px', | |||
| truncateText: false, | |||
| valign: 'middle', | |||
| css: { display: 'block', padding: 0 }, | |||
There was a problem hiding this comment.
Is there anything we can do about retaining the 0 padding here? As it is, the height of the rows increases with more unnecessary whitespace.
Before:
After:
There was a problem hiding this comment.
🤦 Thanks so much for catching that Pete, I see what was happening now. I've restored the original CSS with an extra specificity selector so that the Emotion CSS overrides EUI's not-yet-converted Sass CSS: 60c382a
The TODO is for myself/EUI to remember to remove once EuiTable is converted to Emotion.
There was a problem hiding this comment.
Thanks for making that change @cee-chen
|
Could this also be a solution to the misaligned logo in main?
|
w/ extra nested specificity to fix display
@jughosta Whoa, super weird - this doesn't fix that issue; EuiHeaderLogo's Emotion styles are totally missing. Let me investigate separately 🙏 |
💚 Build SucceededMetrics [docs]Async chunks
Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
|
Thanks y'all! If anyone is coming to this PR after the fact due to unexpected visual bugs from Sass styles coming after Emotion styles in specificity order, please feel free to ping the EUI team for assistance. |
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
Jul 13, 2023
## Summary fixes elastic#161441 fixes elastic#161464 The recent `EuiButtonEmpty`/`EuiButtonIcon` Emotion conversions have highlighted a CSS order/specificity flaw in Kibana compared to EUI - EUI orders its Sass _after_ its Emotion styles (see https://elastic.github.io/eui/#/), and Kibana orders Sass _before_ Emotion styles. I'm not totally sure why Greg set up Kibana's style order the way he did in elastic#134919, but at this point, EUI has enough of its baseline atom components converted to Emotion that remaining components in Sass are all generally molecules or higher level components that need to come after Emotion. ### QA - [x] Test as many pages in Kibana as possible to ensure no visual regressions 🤞 --------- Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> (cherry picked from commit b9eae62)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
12 tasks
This was referenced Jul 20, 2023
cee-chen
added a commit
that referenced
this pull request
Jul 25, 2023
## Summary Follow up to #161592 Some remaining EUI components that are still in Sass unfortunately need to respect EUI's global CSS utilities (e.g. `.eui-yScroll`, `.eui-textTruncate` - [full list here](https://elastic.github.io/eui/#/utilities/css-utility-classes)). Creating a separate utilities cache and insertion point should solve the CSS order/specificity issues. ### Checklist - [x] Confirm Emotion output order is expected in head (EUI globals, All Emotion styles, Sass styles, then utilities last) --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
ThomThomson
pushed a commit
to ThomThomson/kibana
that referenced
this pull request
Aug 1, 2023
## Summary Follow up to elastic#161592 Some remaining EUI components that are still in Sass unfortunately need to respect EUI's global CSS utilities (e.g. `.eui-yScroll`, `.eui-textTruncate` - [full list here](https://elastic.github.io/eui/#/utilities/css-utility-classes)). Creating a separate utilities cache and insertion point should solve the CSS order/specificity issues. ### Checklist - [x] Confirm Emotion output order is expected in head (EUI globals, All Emotion styles, Sass styles, then utilities last) --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
2 tasks
cee-chen
added a commit
that referenced
this pull request
Oct 2, 2024
## Summary This PR removes the `euiFormControlDefaultShadow` mixin from Kibana usage, which is shortly set to be deprecated/removed from EUI. The usages of this mixin primarily wanted the `border` styling of the mixin and not its background effects, so I've opted to simplify the CSS greatly to simply use `border` CSS instead of attempting to mess around with `box-shadow` (which wasn't really benefiting the final visual appearance of the affected use cases). I also incidentally removed some extra CSS specificity added in #156639 (no longer necessary as of #161592) which was causing exclusive borders to not be the correct color. | Before | After | |--------|--------| | <img width="696" alt="" src="https://github.com/user-attachments/assets/00478e77-08e8-490d-90fa-84abd2d3ba99"> | <img width="704" alt="" src="https://github.com/user-attachments/assets/46ef0a5f-5fb0-4d47-82ba-40ed7eb2ff89"> | ### Checklist - [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
Oct 2, 2024
## Summary This PR removes the `euiFormControlDefaultShadow` mixin from Kibana usage, which is shortly set to be deprecated/removed from EUI. The usages of this mixin primarily wanted the `border` styling of the mixin and not its background effects, so I've opted to simplify the CSS greatly to simply use `border` CSS instead of attempting to mess around with `box-shadow` (which wasn't really benefiting the final visual appearance of the affected use cases). I also incidentally removed some extra CSS specificity added in elastic#156639 (no longer necessary as of elastic#161592) which was causing exclusive borders to not be the correct color. | Before | After | |--------|--------| | <img width="696" alt="" src="https://github.com/user-attachments/assets/00478e77-08e8-490d-90fa-84abd2d3ba99"> | <img width="704" alt="" src="https://github.com/user-attachments/assets/46ef0a5f-5fb0-4d47-82ba-40ed7eb2ff89"> | ### Checklist - [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 7edb90e)
kibanamachine
added a commit
that referenced
this pull request
Oct 2, 2024
…es (#194653) (#194730) # Backport This will backport the following commits from `main` to `8.x`: - [Remove Sass `@euiFormControlDefaultShadow` mixin usages (#194653)](#194653) <!--- 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-10-02T15:37:19Z","message":"Remove Sass `@euiFormControlDefaultShadow` mixin usages (#194653)\n\n## Summary\r\n\r\nThis PR removes the `euiFormControlDefaultShadow` mixin from Kibana\r\nusage, which is shortly set to be deprecated/removed from EUI.\r\n\r\nThe usages of this mixin primarily wanted the `border` styling of the\r\nmixin and not its background effects, so I've opted to simplify the CSS\r\ngreatly to simply use `border` CSS instead of attempting to mess around\r\nwith `box-shadow` (which wasn't really benefiting the final visual\r\nappearance of the affected use cases).\r\n\r\nI also incidentally removed some extra CSS specificity added in #156639\r\n(no longer necessary as of #161592) which was causing exclusive borders\r\nto not be the correct color.\r\n\r\n| Before | After |\r\n|--------|--------|\r\n| <img width=\"696\" alt=\"\"\r\nsrc=\"https://github.com/user-attachments/assets/00478e77-08e8-490d-90fa-84abd2d3ba99\">\r\n| <img width=\"704\" alt=\"\"\r\nsrc=\"https://github.com/user-attachments/assets/46ef0a5f-5fb0-4d47-82ba-40ed7eb2ff89\">\r\n|\r\n\r\n### Checklist\r\n\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":"7edb90e8bdc82a338ab8e1c4626bd9bfa69ee3f4","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","v8.16.0","backport:version"],"title":"Remove Sass `@euiFormControlDefaultShadow` mixin usages","number":194653,"url":"https://github.com/elastic/kibana/pull/194653","mergeCommit":{"message":"Remove Sass `@euiFormControlDefaultShadow` mixin usages (#194653)\n\n## Summary\r\n\r\nThis PR removes the `euiFormControlDefaultShadow` mixin from Kibana\r\nusage, which is shortly set to be deprecated/removed from EUI.\r\n\r\nThe usages of this mixin primarily wanted the `border` styling of the\r\nmixin and not its background effects, so I've opted to simplify the CSS\r\ngreatly to simply use `border` CSS instead of attempting to mess around\r\nwith `box-shadow` (which wasn't really benefiting the final visual\r\nappearance of the affected use cases).\r\n\r\nI also incidentally removed some extra CSS specificity added in #156639\r\n(no longer necessary as of #161592) which was causing exclusive borders\r\nto not be the correct color.\r\n\r\n| Before | After |\r\n|--------|--------|\r\n| <img width=\"696\" alt=\"\"\r\nsrc=\"https://github.com/user-attachments/assets/00478e77-08e8-490d-90fa-84abd2d3ba99\">\r\n| <img width=\"704\" alt=\"\"\r\nsrc=\"https://github.com/user-attachments/assets/46ef0a5f-5fb0-4d47-82ba-40ed7eb2ff89\">\r\n|\r\n\r\n### Checklist\r\n\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":"7edb90e8bdc82a338ab8e1c4626bd9bfa69ee3f4"}},"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/194653","number":194653,"mergeCommit":{"message":"Remove Sass `@euiFormControlDefaultShadow` mixin usages (#194653)\n\n## Summary\r\n\r\nThis PR removes the `euiFormControlDefaultShadow` mixin from Kibana\r\nusage, which is shortly set to be deprecated/removed from EUI.\r\n\r\nThe usages of this mixin primarily wanted the `border` styling of the\r\nmixin and not its background effects, so I've opted to simplify the CSS\r\ngreatly to simply use `border` CSS instead of attempting to mess around\r\nwith `box-shadow` (which wasn't really benefiting the final visual\r\nappearance of the affected use cases).\r\n\r\nI also incidentally removed some extra CSS specificity added in #156639\r\n(no longer necessary as of #161592) which was causing exclusive borders\r\nto not be the correct color.\r\n\r\n| Before | After |\r\n|--------|--------|\r\n| <img width=\"696\" alt=\"\"\r\nsrc=\"https://github.com/user-attachments/assets/00478e77-08e8-490d-90fa-84abd2d3ba99\">\r\n| <img width=\"704\" alt=\"\"\r\nsrc=\"https://github.com/user-attachments/assets/46ef0a5f-5fb0-4d47-82ba-40ed7eb2ff89\">\r\n|\r\n\r\n### Checklist\r\n\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":"7edb90e8bdc82a338ab8e1c4626bd9bfa69ee3f4"}},{"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
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
fixes #161441
fixes #161464
The recent
EuiButtonEmpty/EuiButtonIconEmotion conversions have highlighted a CSS order/specificity flaw in Kibana compared to EUI - EUI orders its Sass after its Emotion styles (see https://elastic.github.io/eui/#/), and Kibana orders Sass before Emotion styles.I'm not totally sure why Greg set up Kibana's style order the way he did in #134919, but at this point, EUI has enough of its baseline atom components converted to Emotion that remaining components in Sass are all generally molecules or higher level components that need to come after Emotion.
QA