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] Replace makeStyles/withStyles with sx prop and styled #16947

Closed
84 of 89 tasks
oliviertassinari opened this issue Aug 9, 2019 · 57 comments · Fixed by #32313
Closed
84 of 89 tasks

[docs] Replace makeStyles/withStyles with sx prop and styled #16947

oliviertassinari opened this issue Aug 9, 2019 · 57 comments · Fixed by #32313
Labels
docs Improvements or additions to the documentation priority: important This change can make a difference ready to take Help wanted. Guidance available. There is a high chance the change will be accepted

Comments

@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 9, 2019

Following #6115, I think that we should migrate all our demos to use the Box component or styled-component. The goal is to hide @material-ui/styles as much as possible. styled-components keeps growing, a consolidation of this styling solution will be better, overall, for the React community.

Regarding the timing, I think that we can start to use the Box component now. For the demos that we can't migrate, we probably want to wait for the first proof of concept with #6115.

Attached is a list of component demos that should be migrated to the new style API.

Docs infra

  • AdCarbon.js
  • AdDisplay.js
  • AdReadthedocs.js
  • ComponentLinkHeader.js
  • Demo.js
  • DemoSandboxed.js
  • DemoToolbar.js
  • MarkdownElement.js
  • ErrorDecoder.js
  • Intentions.js
  • Pro.js
  • Quotes.js
  • Sponsors.js
  • Steps.js
  • Users.js
  • TopLayoutCompany.js
  • TopLayoutBlog [docs] Cleanup remaining @mui/styles usages #32313
  • Ad
  • Team

Misc

Layout

Inputs

Navigation

Surfaces

Feedback

Data Display

Utils

Lab

@oliviertassinari oliviertassinari added the docs Improvements or additions to the documentation label Aug 9, 2019
@oliviertassinari oliviertassinari pinned this issue Aug 9, 2019
@yordis
Copy link
Contributor

yordis commented Aug 19, 2019

@oliviertassinari what is the exactly the tasks in hand?

Here is what I understand,

  1. Run the docs website
  2. Find all the example source code that uses material-ui/styles
  3. Replace them with styled-components

Is this correct?

@oliviertassinari oliviertassinari added this to the v5 milestone Aug 22, 2019
@oliviertassinari
Copy link
Member Author

@yordis I think that the timing is going to be key in this transition. I would imagine the following steps:

  1. We replace the usage of @material-ui/styles in the demos with the Box component, where possible. Moving [Box][styled] Cache makeStyles() per JSON of props for style functions #16858 at the same time would help. This can be done in the near future.
  2. We solve Use @material-ui/system in the core components  #15561, we migrate more demos away from @material-ui/styles to use the system props. The sooner we solve this, the better.
  3. We update the customization demos to use styled-components, leveraging the global Mui class names. We might need to work on some helpers to make accessing the theme values easier.
  4. We solve [RFC] Migrate to styled-components #6115, we migrate the last usages of @material-ui/styles to styled-components. This is a task for v5, I think that we can start it Q1 2020 in the v5 alpha/beta versions.

@ConAntonakos
Copy link

ConAntonakos commented Sep 24, 2019

@oliviertassinari I'm curious, and I apologize if this was repeated: why not keep @material-ui/styles as a wrapper or an abstraction for styled-components?

@oliviertassinari
Copy link
Member Author

@ConAntonakos it's an option. It could help if we need to extend/normalize some of the features of styled-components.

@yordis
Copy link
Contributor

yordis commented Oct 20, 2019

@oliviertassinari do we have a list of what's left?

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Oct 20, 2019

@yordis We haven't done many efforts in this direction yet. However, there is a probability that it will require breaking changes, v5 is planned for around Q4 2020. I think that we should explore a partial deployment strategy for developers that want to leverage it sooner. Now, this effort has to be balanced with the other priorities, like the support of new advanced components (free and paid) or the release of an unstyled version of the library to increase our audience reach (with different themes, e.g. iOS style). The good news is that we will likely grow the team in the coming months, enabling us to move faster.

I imagine you are already using styled-components on top of Material-UI today as many other developers do (and not @material-ui/styles). What are the top pain points you hope to address with this migration?

From a product perspective, our incentive is about: smaller bundle size, better performance, steaming support, fewer bugs, CSS template strings support, larger community, enables to use the system props in the core components, allow true dynamic themes and props support, better docs.

@yordis
Copy link
Contributor

yordis commented Oct 20, 2019

However, there is a high probability that it will require breaking changes,

Yeah, I tried to change some examples, but they require some integration with the theme provider, so we may need to inject material/style theme provider and styled-component theme provider I am assuming.

v5 is planned for around Q4 2020.

That is far enough, would it be better to pin different issues for visibility on what is a priority at the moment?

I imagine you are already using styled-components on top of Material-UI today as many other developers do (and not @material-ui/styles).

Actually, I am quite happy with @material-ui/styles and I am not using styled-components when I use Material UI (I would remove withStyles since I don't want to rely on programmer discipline 😉 , but I understand legacy software may no have hooks still )🤷‍♂️

What are the top pain points you hope to address with this migration?

I am trying to help with the migration, personally, no pain points. Unless you take into consideration that as an ecosystem, I have to maintain two ways to develop something, but meh, personally, it is okay for me.

Maybe styled-components fixes some interoperability with NextJS and Gatsby since the last time I tried was hard to make Mui work with those tools because of the SSR and styles (I am not sure if still painful) and most libraries using styled-components work out of the box.

Let me know how I could help, like I said, I was using the pinned issues to find the prioritization of the Org

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Oct 20, 2019

That is far enough, would it be better to pin different issues for visibility on what is a priority at the moment?

It depends on the time horizon you are interested in. You can follow the issue with the label important, the roadmap page on the documentation and the monthly blog product updates.

I don't fully understand this point. Why not using styled-components when using Material-UI (today)? We had 1/3 of our users going down this path when we did our last survey, 6 months ago.

@yordis
Copy link
Contributor

yordis commented Oct 20, 2019

I don't fully understand this point. Why not using styled-components when using Material-UI (today)?

@material-ui/styles works like a champ so far, no reason to migrate everything 😄 so I am one of those out of 3 that don't use it together today.

Thank you for the info about prioritization.

@oliviertassinari oliviertassinari added the priority: important This change can make a difference label Oct 20, 2019
@oliviertassinari
Copy link
Member Author

@yordis btw, my answer was made under the assumption you were following up on the main styled-components issue. I haven't realized we were on the documentation one.

@yordis
Copy link
Contributor

yordis commented Oct 20, 2019

@oliviertassinari do you have any suggestions about the issue with theme context?

I tried to use props.theme inside an styled-components but didn't work, I am not sure if you have a suggestion for it, but I think we will require to add styled-components theme provider as well.

@HiranmayaGundu
Copy link

Hey @oliviertassinari! Are you looking for PR's that convert the existing customization demos to use styled-components as part of this issue?

@nikandlv
Copy link

I dont think styled-components is a good styling solution. current solution looks much much more readable, appealing, accessible and cleaner than styled. i dont see any good reason to migrate.

@oliviertassinari oliviertassinari self-assigned this Feb 2, 2020
@oliviertassinari oliviertassinari added the priority: important This change can make a difference label Feb 2, 2020
@Jack-Works
Copy link
Contributor

I dont think styled-components is a good styling solution. current solution looks much much more readable, appealing, accessible and cleaner than styled. i dont see any good reason to migrate.

And get almost fully typed. Don't see any reason to migrate +1

@oliviertassinari
Copy link
Member Author

oliviertassinari commented May 30, 2021

@thclark, @cjnoname Is the dislike about the styled() API the CSS template string syntax (JavaScript object works too)? Or the love about makeStyles in its capability to have multiple class names (classes object)?

We have talked about bringing makeStyles back powered by emotion, to further ease the migration (we already keep @material-ui/styles around).

@olros
Copy link

olros commented Jun 1, 2021

Is the dislike about the styled() API the CSS template string syntax (JavaScript object works too)? Or the love about makeStyles in its capability to have multiple class names (classes object)?

To me, the best thing about makeStyles is the capability to have muliple class names declared side by side. That allows us to have all the styling used in a component/file declared in a single place and resuse a lot of the styling. In my opinion it does also make much more sense to apply separate styling from the jsx. I think the new demos with the new styled-syntax looks really chaotic and it's difficult to find the code you're looking for when there's styling everywhere.

As a reference, this is an example of makeStyles-usage in a lot my projects:

import classnames from 'classnames';
import { makeStyles } from '@material-ui/styles';

const useStyles = makeStyles((theme) => ({
  grid: {
    display: 'grid',
    gridGap: theme.spacing(2),
    gridTemplateColumns: '1fr 1fr',
    [theme.breakpoints.down('md')]: {
      gridTemplateColumns: '1fr',
      gridGap: theme.spacing(1),
    },
  },
  column: {
    gridTemplateColumns: '1fr',
  },
  margin: {
    margin: theme.spacing(1),
  },
}));

const Example = () => {
  const classes = useStyles();
  return (
    <div className={classnames(classes.grid, classes.margin)}>
      <div className={classnames(classes.grid, classes.column)}>{/* ... */}</div>
      <div className={classnames(classes.grid, classes.column)}>{/* ... */}</div>
    </div>
  );
};

@kelly-tock
Copy link

can totally ask in a separate thread, but realized that the default emotion api is the styled api. will it support using the css api as well?

@cjnoname
Copy link

cjnoname commented Jun 1, 2021

@thclark, @cjnoname Is the dislike about the styled() API the CSS template string syntax (JavaScript object works too)? Or the love about makeStyles in its capability to have multiple class names (classes object)?

We have talked about bringing makeStyles back powered by emotion, to further ease the migration (we already keep @material-ui/styles around).

Thanks for asking.
Personally I hate to wrap many small components with styled'(<></>)', it looks stupid and a lot more work for people to read and write.
Centralize all css into makeStyles would make the CSS code look clean, easy to read and share.

I don't care what's behind "makeStyles". It doesn't matter if it's JSS or Emotion, but "makeStyles" is just a great syntactic sugar I'd like to use.

I am pretty sure that a lot of MUI users are moved from Style Component in order to be avoid of working with 'styles()' syntax.

@PilotConway
Copy link

We really like makeStyles on our team as well for two reasons. The first is the multiple classes in a single object and the second is being able to keep the styling separate from the layout. And we always know where styles are since they are defined at the top of our files (the order in our component files is always the same which is nice).

I agree as well, styled components are quick messy in a lot of cases. I'm not opposed to using them when I'm trying to wrap and re-export a single component, but when I'm just trying to apply a few additional styles to my JSX I vastly prefer the makeStyles hook syntax.

@thclark
Copy link

thclark commented Jun 1, 2021

@thclark, @cjnoname Is the dislike about the styled() API the CSS template string syntax (JavaScript object works too)? Or the love about makeStyles in its capability to have multiple class names (classes object)?

We have talked about bringing makeStyles back powered by emotion, to further ease the migration (we already keep @material-ui/styles around).

@oliviertassinari my main dislike is the template string syntax. I know that styled() does work with javascript objects however the overwhelming majority of examples and tutorials out there don't use it. So we either accept dumping weird raw css strings everywhere (which are also then agony to template-in variables like logic from the theme) or you find yourself endlessly converting strings into JSS in order to copy-paste something into your code.

Perhaps, in that light, if the mui docs adopted a convention of using JSS in all the styled examples, that would be one way of easing the transition from makestyles.

@kelly-tock
Copy link

one thing I miss ATM is being able to run stylelint on the makeStyles code. i'm assuming using regular css syntax will allow us to do that with this new approach? it seems like emotion has stylelint support already?

@nathanlambert
Copy link

nathanlambert commented Jun 2, 2021

@kelly-tock I do this to get linting on standard CSS (also IIRC soon experimentalStyled will be renamed styled):

import { css } from "@emotion/react";
import { Box, experimentalStyled as styled } from "@material-ui/core";

const MyBox = styled(Box)(
  ({ theme }) => css`
    display: flex;
    margin-top: ${theme.spacing(2)};
    padding: ${theme.spacing(2)};
  `
);

@oliviertassinari
Copy link
Member Author

I have created #26571 to have a place for the community to contribute to the pain generated by putting makeStyles on the side going forward with v5.

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 priority: important This change can make a difference ready to take Help wanted. Guidance available. There is a high chance the change will be accepted
Projects
None yet
Development

Successfully merging a pull request may close this issue.