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] Improve documentation of the system #23294

Merged
merged 90 commits into from
Nov 12, 2020

Conversation

mnajdova
Copy link
Member

@mnajdova mnajdova commented Oct 28, 2020

This PR improves the documentation around the system, extending the basics page to include more information about the motivation of the system and the sx prop.

Preview: https://deploy-preview-23294--material-ui.netlify.app/system/basics/.

@mui-pr-bot
Copy link

mui-pr-bot commented Oct 28, 2020

Details of bundle changes

Generated by 🚫 dangerJS against 5aa6192

@eps1lon
Copy link
Member

eps1lon commented Oct 28, 2020

Why do we need a separate page if all the existing props are deprecated anyway? I don't think we should continue documentation of deprecated props.

@oliviertassinari oliviertassinari added the docs Improvements or additions to the documentation label Oct 28, 2020
@mnajdova
Copy link
Member Author

Why do we need a separate page if all the existing props are deprecated anyway? I don't think we should continue documentation of deprecated props.

This is a document for the sx which is not deprecated prop. The other pages in the system are documenting the functions for each of the system props - which are used inside the sx or can be used as standalone styling function.

Are you referring to the other Box props, which are deprecated now?

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.

Should we move https://deploy-preview-23294--material-ui.netlify.app/system/basics/#responsive to this new sx page?

I wonder if we should reconsider the whole documentation for the system. Thinking about it from a holistic perspective, where sx is one part of it.

docs/src/pages/system/sx/sx.md Outdated Show resolved Hide resolved
docs/src/pages/system/sx/sx.md Outdated Show resolved Hide resolved
docs/src/pages.js Outdated Show resolved Hide resolved
docs/src/pages/system/sx/sx.md Outdated Show resolved Hide resolved
docs/translations/translations.json Outdated Show resolved Hide resolved

## Getting Started

Each `@material-ui/core` component supports the `sx` prop. The prop is a superset of CSS, that let's you add any CSS to the underlaying component, while mapping values to different theme keys. This should help you to keep your application look consistent.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I would start with mentioning @material-ui/core.

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you think of this:

The sx prop is a superset of CSS, that let's you add any CSS to the underlaying component, while mapping values to different theme keys. This should help you to keep your application look consistent. The prop is available in all @material-ui/core components.

Later when we support this on all primitives, we can add that here too. Or at least, we can export the styleFunctionSx and add something like:

You can include this prop on your own components too by using the styleFunctionSx style function from @material-ui/core/Box, or you can automatically add it to your component by using the experimentalStyled from @material-ui/core/styles.

import { styleFunctionSx } from '@material-ui/core/Box';
import styled from '@emotion/styled';

const Div = styled('div')(styleFunctionSx);

or

import { experimentalStyled as styled } from '@material-ui/core/styles';

const Div = styled('div')``;

Copy link
Member

Choose a reason for hiding this comment

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

I'l wait for it to be updated before correcting this.

Copy link
Member

Choose a reason for hiding this comment

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

We haven't decided on adding sx to every component nor have we released it. We shouldn't document something before.

Copy link
Member

@oliviertassinari oliviertassinari Oct 29, 2020

Choose a reason for hiding this comment

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

We haven't decided on adding sx to every component nor have we released it. We shouldn't document something before.

The Slider is meant to be a projection of how all the other components will be architectured. Slider now supports the sx prop and was released. So I think that in theory, this decision and release event has already happened. I would argue that the decision started 2 years ago with

In the future, we will look into using it directly in the core components.

https://medium.com/material-ui/introducing-material-ui-design-system-93e921beb8df

"look into" as how can we make it fast enough. Then about a year ago with #15561.

Having a look at all the issues that where closed linking to this issue (#15561) is interesting to gauge the pain.

Copy link
Member

Choose a reason for hiding this comment

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

So I think that in theory, this decision and release event has already happened.

That is not how an alpha works. Its goal is to give us freedom to add and remove things without having to release a major version every time. If you're now flipped to "everything released in alpha is stable" then you should've told us that before we started working on v5. That's a pretty fundamental change to the alpha process especially if it involves decisions we haven't discussed.

As ususal: something existing is in no way shape or form under any circumstance and argument for its existence. Slider with sx is not a good API just because it shipped with it. The original PR still doesn't include a real world use case and just some toying around.

I would argue that the decision started 2 years ago with

A decision isn't a process. Could you link me the conclusion of the discussion with having widgets with system props that just affect the root container?

Copy link
Member

Choose a reason for hiding this comment

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

Its goal is to give us freedom to add and remove things without having to release a major version every time.

Agree, the objective of pre-releases is to allow a short feedback loop. It allows us to validate ideas directly with developers. On this one, we are waiting to get insight from developers after having released it. It seems that the trend of improvement is toward supporting sx everywhere: #23220. The value of the prop increases the more it can be used in a systematic manner.


I don't understand the concern with the sx prop, the value proposition started to be discussed in #22819, then #23053, e.g. #23053 (comment).

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand the concern with the sx prop

It's JavaScript. JavaScript is only good if it solves a problem. On the flipside: I do not understand how you don't see a problem with adding features after maintaining open-source for 3+ years.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, it makes sense, so I would summarize the concern into: is this going in the right direction, and is the opportunity cost right? From my perspective, the answer is yes to both of these questions. I would personally use it over what we have now in future projects. It's really meant to be Bootstrap's utility equivalent (but covering more use cases).

I guess we can keep it in the alpha state longer to make sure we don't regret it?

mnajdova and others added 5 commits October 28, 2020 17:26
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
docs/src/pages/system/sx/BreakpointsAsArray.tsx Outdated Show resolved Hide resolved
docs/src/pages/system/sx/BreakpointsAsObject.js Outdated Show resolved Hide resolved
docs/src/pages/system/sx/BreakpointsAsArray.js Outdated Show resolved Hide resolved
docs/src/pages/system/sx/BreakpointsAsObject.tsx Outdated Show resolved Hide resolved
docs/src/pages/system/sx/GettingStarted.js Outdated Show resolved Hide resolved
docs/src/pages/system/sx/sx.md Outdated Show resolved Hide resolved
docs/src/pages/system/sx/sx.md Outdated Show resolved Hide resolved
docs/src/pages/system/sx/sx.md Outdated Show resolved Hide resolved
docs/src/pages/system/sx/sx.md Outdated Show resolved Hide resolved
docs/src/pages/system/sx/sx.md Outdated Show resolved Hide resolved
mnajdova and others added 8 commits October 28, 2020 19:48
Co-authored-by: Matt <github@nospam.33m.co>
Co-authored-by: Matt <github@nospam.33m.co>
Co-authored-by: Matt <github@nospam.33m.co>
Co-authored-by: Matt <github@nospam.33m.co>
Co-authored-by: Matt <github@nospam.33m.co>
Co-authored-by: Matt <github@nospam.33m.co>
Co-authored-by: Matt <github@nospam.33m.co>
Co-authored-by: Matt <github@nospam.33m.co>
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 10, 2020
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 11, 2020
@mnajdova
Copy link
Member Author

@mbrookes @eps1lon let's do a final review on this one 🙏

@eps1lon
Copy link
Member

eps1lon commented Nov 11, 2020

Are the benchmark changes supposed to be part of this PR?

Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

Looks good overall. Two remaining issues.

The code preview below the demo in https://deploy-preview-23294--material-ui.netlify.app/system/basics/#demo is not showing the default export but some helper component.

docs/src/pages/system/basics/Why.js Outdated Show resolved Hide resolved
@mnajdova
Copy link
Member Author

Are the benchmark changes supposed to be part of this PR?

I see they are added with 5514a6b @oliviertassinari should we extract these changes as a separate PR?

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 11, 2020
@eps1lon
Copy link
Member

eps1lon commented Nov 12, 2020

Fixing the PR to get it in a mergable state to unblock #23285.

Benchmarks should be part of another PR anyway and dropping prettier can be discussed in a separate issue. We shouldn't discuss this in a side-conversation. Prettier was a huge step for the ecosystem and I don't want to see it removed in such a handwavy manner.

@eps1lon
Copy link
Member

eps1lon commented Nov 12, 2020

Are the benchmark changes supposed to be part of this PR?

I see they are added with 5514a6b @oliviertassinari should we extract these changes as a separate PR?

Benchmarks make sense with that context. Leaving them in

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 12, 2020
@eps1lon
Copy link
Member

eps1lon commented Nov 12, 2020

Merging. We'll have another follow-up PR (#23294 (comment)) where we can discuss remaining issues. It's more important to get rid of deprecated props in the docs first.

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.

5 participants