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] Remove use of StyledEngineProvider at the top of the App #27358

Conversation

mnajdova
Copy link
Member

@mnajdova mnajdova commented Jul 19, 2021

Updates done in the PR:

  • removes the use of <StyledEngineProvider injectFirst> at the top of the app
  • uses custom cache provider only if rtl is on
  • uses enhanceApp in favor of enhanceComponent

Tested:

  • rtl is working
  • dark mode is working
  • global styles are not leaking

Preview: https://deploy-preview-27358--material-ui.netlify.app/

@mui-pr-bot
Copy link

mui-pr-bot commented Jul 19, 2021

No bundle size changes (experimental)

Generated by 🚫 dangerJS against 7550794

@mnajdova
Copy link
Member Author

We need to first not have any makeStyles usages on the docs in order for this to work.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 20, 2021

Does it mean we can close #16947? cc @vicasas Are we done? I'm not sure

@vicasas
Copy link
Member

vicasas commented Jul 20, 2021

Are we done?

@oliviertassinari From what I remember, some module files are missing from the onepirate template. And all the others that don't have a check.

@mnajdova
Copy link
Member Author

Does it mean we can close #16947? cc @vicasas Are we done? I'm not sure

There are few occurrences, I will remove them as part of this PR

@oliviertassinari
Copy link
Member

There are few occurrences, I will remove them as part of this PR

@mnajdova I had a quick look. I could find the following components that are using JSS and that override emotion:

  • Pro
  • Quotes
  • Sponsors
  • Steps
  • Users
  • TopLayoutCompany
  • TopLayoutBlog
  • Ad
  • ReactVirtualizedTable
  • Team
  • Onepirate/Snackbar
  • Onepirate/TextField
  • Onepirate/Typography

Regarding the broken search bar and drawer, it looks like this PR is not based on HEAD, so likely why we have issues.

@oliviertassinari oliviertassinari added docs Improvements or additions to the documentation v5.x migration labels Jul 24, 2021
@mbrookes mbrookes changed the title [docs] Remove StyledEngineProvider usage on the top of the App [docs] Remove use of StyledEngineProvider at the top of the App Jul 24, 2021
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 28, 2021
@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 1, 2021

@mnajdova Do you plan to resume the work on this PR or will it be delayed to post v5? If it's later, I propose we close. We can use the GitHub issues to keep track of the work that is left to be done #16947.


As a general note (not directly related to this PR, but it could be). We don't really have to use PRs as a personal bookmark, it could be done like on Google Keep, Evernote, etc. Would it make sense to enforce an automatic PR close logic for PRs inactive after, say 30 days, more? Maybe it would set a healthy constraint to stay in sync with what we wish our bandwidth would be, what its in reality, and what our true priorities are?
It would close all PRs that we have deprioritized (inactive for a while), it would lead to having the open PRs for ongoing work. My assumption is that it could 1. make it reviewing work more appealing (why? to know where to focus). 2. communicate to others what's up for grab (why? seeing something open doesn't open somebody else to work on the same problem).
When I look at vscode with 254 open PRs, Flutter with 168 open PRs, TypeScript with 273, I can't help myself thinking. How much of them are really relevant?

@mnajdova
Copy link
Member Author

mnajdova commented Aug 2, 2021

@mnajdova Do you plan to resume the work on this PR or will it be delayed to post v5? If it's later, I propose we close. We can use the GitHub issues to keep track of the work that is left to be done #16947.

We can close, I planned to work on it last week, but other things came up. I will re-open once I get back to this, or start from scratch...

As a general note (not directly related to this PR, but it could be). We don't really have to use PRs as a personal bookmark, it could be done like on Google Keep, Evernote, etc. Would it make sense to enforce an automatic PR close logic for PRs inactive after, say 30 days, more? Maybe it would set a healthy constraint to stay in sync with what we wish our bandwidth would be, what its in reality, and what our true priorities are?

Agree, it makes sense. If someone haven't work on a PR for a month, it's likely that we can close.

@mnajdova mnajdova closed this Aug 2, 2021
@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 7, 2021

A fun one in the OSS landscape https://github.com/odoo/odoo, more PRs open than issues 😁

Capture d’écran 2021-08-07 à 22 01 07

I have open #16947

If someone haven't work on a PR for a month, it's likely that we can close.

To see if this is the right threshold, could be higher, like 3 months. But I think that there is a notion of: I don't spend time on X === it's not important enough or it's blocked by something else => close.

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 PR: out-of-date The pull request has merge conflicts and can't be merged v5.x migration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants