[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
cee-chen
force-pushed
the
emotion/css-prefixes
branch
from
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`
cee-chen
force-pushed
the
emotion/css-prefixes
branch
from
612b092 to
9d4bb78
Compare
|
Preview staging links for this PR:
|
💚 Build Succeeded
History
|
| "@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.
@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.
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.
1Copenut
left a comment
1Copenut
left a comment
There was a problem hiding this comment.
👍 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))
4 participants
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)-