Skip to content
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

Merged
merged 4 commits into from
Jan 26, 2022
Merged

Conversation

ryancogswell
Copy link
Contributor

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.

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.

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.
@ryancogswell
Copy link
Contributor Author

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.

@danilo-leal danilo-leal added the docs Improvements or additions to the documentation label Jan 20, 2022
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

docs/src/pages/guides/right-to-left/right-to-left.md Outdated Show resolved Hide resolved
From Olivier's suggested change

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
@mui-bot
Copy link

mui-bot commented Jan 20, 2022

No bundle size changes

Generated by 🚫 dangerJS against 249b542

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 20, 2022

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:

sUb14

with no gain.

@oliviertassinari oliviertassinari added the package: system Specific to @mui/system label Jan 21, 2022
@Andarist
Copy link
Contributor

@Andarist Do you think that stylis would be up to remove some of the prefixes?

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.

@oliviertassinari
Copy link
Member

I have pushed another commit, to fix all the instances in our codebase. I have also opened thysultan/stylis#283.

@thysultan
Copy link

In a way, this could be seen as a breaking change.

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 export prefixer_bleeding_age™ given that the current prefixer is not privileged and is as much a middleware as any other.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 25, 2022
@mnajdova
Copy link
Member

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.

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

Copy link
Member

@mnajdova mnajdova left a 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 👍

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 25, 2022
@mnajdova mnajdova merged commit ef6ffb0 into master Jan 26, 2022
@mnajdova mnajdova deleted the ryancogswell-patch-1 branch January 26, 2022 10:56
wladimirguerra pushed a commit to wladimirguerra/material-ui that referenced this pull request Feb 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation package: system Specific to @mui/system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants