-
Notifications
You must be signed in to change notification settings - Fork 831
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] Reduce CSS browser prefixing to supported browsers only #7272
Conversation
- not needed by latest evergreen browsers
- not needed by evergreen browsers - https://caniuse.com/mdn-css_at-rules_keyframes
- more useful to have devs see this immediately, particularly the comment
- unnecessary for evergreen browsers
- other values appear to be unnecessary, per linked caniuse docs
+ reorder + fix `print-color-adjust` property, `color-adjust` does not exist - also who on earth is using this
- only 1 value needs it
b17be1f
to
612b092
Compare
…xer plugin - this requires creating a new custom vanilla JS `@emotion/css` cache - NOTE: Other components that use `@emotion/css` (e.g. overlay mask, code block) should NOT use this new util yet, as there's a bug with its auto label behavior
+ fix jsdoc typo
The removed `colorClassName` auto label is due to the difference in how the default `@emotion/css` behaves compared to `@emotion/css/create-instance`
612b092
to
9d4bb78
Compare
Preview staging links for this PR:
|
💚 Build Succeeded
History
|
@@ -150,6 +150,7 @@ | |||
"@types/react-dom": "^18.2.6", | |||
"@types/react-is": "^17.0.3", | |||
"@types/react-router-dom": "^5.3.3", | |||
"@types/stylis": "^4.2.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tkajtoch I'd appreciate your advice on this one - the stylis
package that we're using to write a custom prefixer plugin for Emotion is a dependency of @emotion/css
and thus should always be present in consumer applications (since @emotion/css
is listed in peerDependencies
). I'm hesitating as to whether or not that means we need to explicitly list the stylis
dependency in our own package.json, and if we do, whether it should be under either dependencies
or peerDependencies
.
What's also a little odd is that Emotion appears to pin its dependency on stylis
specifically at 4.2.0 without a caret (latest is 4.3.0), so I worry that including our own stylis version would potentially cause weird issues or conflicts with Emotion. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Chatted with Tomasz on this in our sync today - he's good with moving forward with this. If any issues do come up with consumer dependencies, we can absolutely revisit/fix this in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 LGTM. I'm interested to hear @tkajtoch input on the package.json
change; I don't have a deep enough understanding of dependency management to chime in on this item.
I read through the commits singularly and the changeset as a whole to get a better depth and breadth understanding. This is a big improvement in reducing unneeded CSS prefixes.
I noticed a typo in one of the links in docs (not in your changeset) that I'll fix and PR quick with reference to this PR.
`v89.0.0`⏩`v89.1.0` This upgrade also contains an EuiDataGrid refactor (elastic/eui#7255) not listed in the changelog (as no end-user functionality or props changed as a result of the refactor). The unlisted changes should only affect DOM and `className` usages in Kibana (primarily CSS overrides and test selectors). --- ## [`89.1.0`](https://github.com/elastic/eui/tree/v89.1.0) - Added `tokenVectorSparse` token and updated `tokenDenseVector` token (now named `tokenVectorDense`). ([#7282](elastic/eui#7282)) **CSS-in-JS conversions** - Reduced default CSS prefixes generated by Emotion to only browsers supported by EUI (latest evergreen browsers). This can be customized by passing your own Emotion cache to `EuiProvider`. ([#7272](elastic/eui#7272))
`v89.0.0`⏩`v89.1.0` This upgrade also contains an EuiDataGrid refactor (elastic/eui#7255) not listed in the changelog (as no end-user functionality or props changed as a result of the refactor). The unlisted changes should only affect DOM and `className` usages in Kibana (primarily CSS overrides and test selectors). --- ## [`89.1.0`](https://github.com/elastic/eui/tree/v89.1.0) - Added `tokenVectorSparse` token and updated `tokenDenseVector` token (now named `tokenVectorDense`). ([elastic#7282](elastic/eui#7282)) **CSS-in-JS conversions** - Reduced default CSS prefixes generated by Emotion to only browsers supported by EUI (latest evergreen browsers). This can be customized by passing your own Emotion cache to `EuiProvider`. ([elastic#7272](elastic/eui#7272))
`v89.0.0`⏩`v89.1.0` This upgrade also contains an EuiDataGrid refactor (elastic/eui#7255) not listed in the changelog (as no end-user functionality or props changed as a result of the refactor). The unlisted changes should only affect DOM and `className` usages in Kibana (primarily CSS overrides and test selectors). --- ## [`89.1.0`](https://github.com/elastic/eui/tree/v89.1.0) - Added `tokenVectorSparse` token and updated `tokenDenseVector` token (now named `tokenVectorDense`). ([elastic#7282](elastic/eui#7282)) **CSS-in-JS conversions** - Reduced default CSS prefixes generated by Emotion to only browsers supported by EUI (latest evergreen browsers). This can be customized by passing your own Emotion cache to `EuiProvider`. ([elastic#7272](elastic/eui#7272))
Summary
I noticed the other day that our Emotion styles are outputting a lot of browser prefixes we don't need (that even our Sass doesn't have), and they're for browsers that EUI/Elastic do not support (see Elastic's support matrix, which is essentially "latest evergreen browsers"). It appears Greg noticed this too about a year ago, but didn't have bandwidth to address it during the original CSS-in-JS setup work 🥲 #5670 (review)
Related reading:
QA
It isn't terribly reasonable to smoke test the entire EUI app (again, this is where visual regression tests would have really given us confidence!) - we have to hope these changes are reasonable, and that unit tests / Kibana QA will catch any final issues
General checklist
- [ ] Checked in mobile- [ ] Checked for accessibility including keyboard-only and screenreader modesand cypress tests- [ ] If applicable, added the breaking change issue label (and filled out the breaking change checklist)-