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 makeStyles from landing pages #26130

Merged
merged 8 commits into from
May 7, 2021

Conversation

mnajdova
Copy link
Member

@mnajdova mnajdova commented May 4, 2021

@mui-pr-bot
Copy link

mui-pr-bot commented May 4, 2021

No bundle size changes

Generated by 🚫 dangerJS against 45d7498

@oliviertassinari oliviertassinari 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 May 4, 2021
@@ -1,30 +1,27 @@
import * as React from 'react';
import { makeStyles } from '@material-ui/core/styles';
import { experimentalStyled as styled } from '@material-ui/core/styles';
Copy link
Member

Choose a reason for hiding this comment

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

Why are we using experimentalStyled in these demos? All the functionality it provides over styled is unused here.

Copy link
Member

Choose a reason for hiding this comment

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

So far, we have never used the styled API exported by @material-ui/styled-engine directly in the docs or the components.

I think that If we use both, developers will start to confuse the two (as they did in v4 when we documented the two, I can find the GitHub issues about it if needed, ultimately, we reverted).

I personally don't see the value of using the styles helper from @material-ui/styled-engine directly unless you are a third party library that wants to benefit from emotion/sc adaptation.

Copy link
Member

@eps1lon eps1lon May 5, 2021

Choose a reason for hiding this comment

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

So far, we have never used the styled API exported by @material-ui/styled-engine directly in the docs or the components.

A descriptive fact but not a normative argument. In other words, you're saying what we are doing not arguing for what we ought to do.

I think that If we use both, developers will start to confuse the two (as they did in v4 when we documented the two, I can find the GitHub issues about it if needed, ultimately, we reverted).

The old makeStyles had the same implementation with the same name but different themes. That's something you are confused about.

You're importing two different implementations with different interfaces and different names. Is this an argument you want to make? Just so that I know in case you start arguing that we should remove DesktopTimePicker and MobileTimePicker because people used to confuse makeStyles 😕

I personally don't see the value of using the styles helper from @material-ui/styled-engine directly unless you are a third party library that wants to benefit from emotion/sc adaptation.

Oh, so we are in a "micro perf does not matter" arc now. How long are you planning to use that argument? Just so that I know when to argue on or the other. I guess once you start working on benchmarks again it'll become important. In the meantime it's not an argument because you're using experimentalStyled. Once you start using styled you'll likely argue that it's faster.

Copy link
Member Author

Choose a reason for hiding this comment

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

styled() from @material-ui/styled-engine does not provide default theme, and this is one of the biggest reason why we should not use it in the docs. We want developers to have zero-to-configure setup, if we start replacing experimentalStyled() with styled() we would loose that, as they would have to have ThemeProvider on the top of their app.

Copy link
Member Author

@mnajdova mnajdova May 5, 2021

Choose a reason for hiding this comment

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

I should have probably stated this argument first.

Copy link
Member

Choose a reason for hiding this comment

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

So we want to add a functionality that provides the default argument.

Because experimentalStyled as the go to for developers is just not acceptable. I don't know why we think adding all that unused code to the bundle is acceptable. Like, I'm not saying that experimentalStyled is not useful. Or bad code. But all that JS is just not needed.

We're exporting various components for that reason. So that people can choose what they need. We don't export a single Box just so that we can say: "Ah, look at all these consistent imports. So beautiful."

Consistency is not an argument. It's just a descriptive fact about a "thing". Consistency itself has no inherent value. You wouldn't accept that in 99% of arguments. For example, I think we should import everything from @material-ui/all for consistency. I think we should name every function do for consistency. I think we should write everything in ES3 for consistency. Consistency solves a problem. That problem is the argument. Not consistency.

So the problem here is that we only have experimentalStyled with a default theme. But experimentalStyle is just overloaded for 99% of the use cases. That problem we need to solve. If we care about user experience. Because that is impacted seriously here.

Copy link
Member

Choose a reason for hiding this comment

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

Consistency is not an argument. It's just a descriptive fact about a "thing"

Agree 💯, same for being DRY by the way. For me, it's rather that inconsistencies suggest that we might solve the same problem in two different ways, which could be a lost opportunity. A lost opportunity for having to fix the same bugs twice in different ways, or having to learn almost the same thing twice.

Because experimentalStyled as the go to for developers is just not acceptable.

I think that we should go back to the fundamental. What problem we are trying to solve? The experimentalStyled module adds the following:

  1. default theme
  2. sx prop
  3. forward prop logic
  4. add debuggable class names
  5. theme overrides, theme variants

From what I understand, we have two distinct use cases for the styled helper:

  • Developers that are extending our core components for their design system. They need all of the 5. points, like they needed the feature in v4. It's important so they can truly extend it and provide the same DX between core and extended components to the developers using their design system.
  • Developers that are only styling for a specific case. They only need 1. 2. (sometimes). Now, 3. 4. 5. are from what I believe bypassed at runtime, and the code is already bundled no matter what as soon as one core component is important, so I fail to see the win. All I can see is the cost of documenting two modules (having to teach the difference, when to one or the other).

Copy link
Member Author

Choose a reason for hiding this comment

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

So the problem here is that we only have experimentalStyled with a default theme. But experimentalStyle is just overloaded for 99% of the use cases. That problem we need to solve. If we care about user experience. Because that is impacted seriously here.

We can discuss this separately, I would argue that having options in this utility would be easier for the learning curve, as developers can always use the same util if they want to style something. Not sure that the bundle overhead is too big to make this undesirable. Overall I think we should push with using experimentalStyled both internally and externally at this moment, as it is the best API we have. If we come up with a better API, we can adjust it later on.

Copy link
Member Author

Choose a reason for hiding this comment

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

@eps1lon @oliviertassinari is there anything else on this one, or can I merge it?

mnajdova and others added 4 commits May 5, 2021 10:50
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
@mnajdova mnajdova merged commit 1654d44 into mui:next May 7, 2021
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 this pull request may close these issues.

None yet

4 participants