-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
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
[docs] Retain vendor prefixing in rtl example #30710
Conversation
The `prefixer` stylis plugin is included by default when not overriding `stylisPlugins` via `CacheProvider`. When explicitly specifying `stylisPlugins` in order to add the rtl plugin, `prefixer` needs to be included in order to retain the default vendor prefixing behavior.
I suspect that the "4.2 styled-components" section may need a similar change, but I haven't done any testing to see if that example also results in prefixing being turned off. |
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.
From Olivier's suggested change Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
I see more places in the codebase that should be updated: 1. the demo sandbox, 2. the docs wrapper, 3. the Directions demo. For those that wonder, do we still need this vendor prefix logic? Yes, for props like background-clip. @Andarist Do you think that stylis would be up to remove some of the prefixes? For instance, flex-shrink, really? I would argue that these flex prefixes harms the DX: with no gain. |
In a way, this could be seen as a breaking change. I'm not sure what's Sultan's view on the matter though. I think it's worth creating an issue there and I agree that the current set of prefixed style declarations is overly broad. If this suggestion would be rejected we can also discuss creating a slimmed-down prefixed for Emotion and use that instead (but I would also have to recheck with Mitchell to see what he thinks about it). It would be great if somebody could list what gets currently prefixed and assess how useful each thing is. |
I have pushed another commit, to fix all the instances in our codebase. I have also opened thysultan/stylis#283. |
In a way you could avoid a breaking change by exporting a prefixer, that is a 1-1 clone of the current prefixer with the suggested amendments, i.e |
As far as I could see, the StyleSheetManager has an option for disabling prefixer, if it is not disabled, the prefixer is always added - https://github.com/styled-components/styled-components/blob/80cf751528f5711349dd3c27621022b4c95b4b7f/packages/styled-components/src/utils/stylis.ts#L76 |
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.
The changes looks good. Great finding 👍
The
prefixer
stylis plugin is included by default when not overridingstylisPlugins
viaCacheProvider
. When explicitly specifyingstylisPlugins
in order to add the rtl plugin,prefixer
needs to be included in order to retain the default vendor prefixing behavior.This came to my attention due to looking into how to turn off the vendor prefixing for a StackOverflow question (https://stackoverflow.com/questions/70656499/material-ui-disable-browser-specific-css-rules-webkit-etc) and then realizing that the rtl instructions currently also cause vendor prefixing to be turned off.