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

Documentation of makeStyles #17387

Closed
mnemanja opened this issue Sep 11, 2019 · 6 comments · Fixed by #17447
Closed

Documentation of makeStyles #17387

mnemanja opened this issue Sep 11, 2019 · 6 comments · Fixed by #17447
Labels
docs Improvements or additions to the documentation package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5.

Comments

@mnemanja
Copy link
Contributor

In the documentation, the dependency import of makeStyles API page states that it should be imported from @material-ui/styles:
import { makeStyles } from '@material-ui/styles';

But on the button page, it says it should be taken from the '@material-ui/core/styles';
import { makeStyles, createStyles, Theme } from '@material-ui/core/styles';

For my test to work I'm using the latter one.
Can you please unify the information in the documentation?

@mbrookes
Copy link
Member

mbrookes commented Sep 11, 2019

Agreed, it's quite confusing at the moment.

From my understanding, we should remove all references to @material-ui/styles, except a footnote somewhere that you can use this path if using the styles package without @material-ui/core being installed (0.00001% of cases*). @oliviertassinari correct me here otherwise.

* Not a real statistic.

@mbrookes mbrookes added docs Improvements or additions to the documentation package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5. labels Sep 11, 2019
@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 11, 2019

Should we close as a duplicate of #15867?

we should remove all references to @material-ui/styles

@mbrookes This would work for the duplicated modules. What about the unique modules, only exported from @material-ui/styles, e.g. the SSR API?

I don't think that we should, nor try to, develop our own styling solution for the sake of it. I think that we should focus on what we do best: build higher-level abstractions (components) on top of the already popular styling solutions.

As far as I know, people are using our styling solution for the following reasons, by order of importance:

  1. It comes by default with the existing components. This means fewer configuration, fewer conflict, fewer bundle size ([RFC] Migrate to styled-components #6115)
  2. It's documented in all the demos, developers can copy and paste, it will just work ([docs] Replace makeStyles/withStyles with sx prop and styled #16947)
  3. ?

@mbrookes
Copy link
Member

I don't think that we should, nor try to, develop our own styling solution for the sake of it. I think that we should focus on what we do best

But we have done already (or rather extended JSS), and documented it along side Material-UI, with two different ways to use it, which is an ongoing source of confusion.

How to minimise the confusion? Only document the one import path for the common use case (i.e. use with Material-UI).

What about the unique modules, only exported from @material-ui/styles, e.g. the SSR API?

Either we export it from core/styles, or document the styles import in the SSR guide (with the API documented in the package README). It's a more advanced use case with a smaller number of users.

#6115, #16947

That's a 9 months away. In the meantime, we can improve the situation.

@oliviertassinari
Copy link
Member

How to minimise the confusion?

For the next 9 months, only documenting imports from @material-ui/core and explaning that styles can be used too would probably be the best tradeoff. So yes, I think that we can re-export everything from styles in core. It would remove the need for a manual installation of the styles package and avoid potential duplication of the module in people bundles :).

@mbrookes
Copy link
Member

@mnemanja Thanks for raising the issue. Would you like to work on it? I can help with any docs wording...

@mnemanja
Copy link
Contributor Author

@mbrookes Sure thing. I'll address it asap.

mnemanja pushed a commit to mnemanja/material-ui that referenced this issue Sep 16, 2019
mnemanja pushed a commit to mnemanja/material-ui that referenced this issue Sep 16, 2019
mbrookes pushed a commit to mnemanja/material-ui that referenced this issue Sep 28, 2019
mbrookes pushed a commit to mnemanja/material-ui that referenced this issue Sep 28, 2019
oliviertassinari pushed a commit to mnemanja/material-ui that referenced this issue Oct 1, 2019
oliviertassinari pushed a commit to mnemanja/material-ui that referenced this issue Oct 1, 2019
oliviertassinari pushed a commit to mnemanja/material-ui that referenced this issue Oct 3, 2019
oliviertassinari pushed a commit to mnemanja/material-ui that referenced this issue Oct 3, 2019
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: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants