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

[Joy] Add Sheet doc #32820

Merged
merged 22 commits into from Jun 22, 2022
Merged

[Joy] Add Sheet doc #32820

merged 22 commits into from Jun 22, 2022

Conversation

hbjORbj
Copy link
Member

@hbjORbj hbjORbj commented May 18, 2022

@hbjORbj hbjORbj marked this pull request as draft May 18, 2022 12:44
@hbjORbj hbjORbj added the package: joy-ui Specific to @mui/joy label May 18, 2022
@hbjORbj hbjORbj self-assigned this May 18, 2022
@hbjORbj hbjORbj added the docs Improvements or additions to the documentation label May 18, 2022
@mui-bot
Copy link

mui-bot commented May 18, 2022

Details of bundle changes

Generated by 🚫 dangerJS against 9ca3ca0

@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels May 20, 2022
@hbjORbj hbjORbj marked this pull request as ready for review June 2, 2022 10:40
@hbjORbj hbjORbj requested a review from siriwatknp June 2, 2022 10:40
@hbjORbj hbjORbj requested a review from danilo-leal June 2, 2022 10:41
@hbjORbj
Copy link
Member Author

hbjORbj commented Jun 2, 2022

@danilo-leal @siriwatknp
What do you think about changing the default variant value to either outlined / soft / solid? plain doesn't seem to showcase the component well enough: https://deploy-preview-32820--material-ui.netlify.app/joy-ui/react-sheet/

@hbjORbj hbjORbj requested a review from siriwatknp June 3, 2022 09:26
@siriwatknp
Copy link
Member

@danilo-leal @siriwatknp What do you think about changing the default variant value to either outlined / soft / solid? plain doesn't seem to showcase the component well enough: https://deploy-preview-32820--material-ui.netlify.app/joy-ui/react-sheet/

Added "bg": true to the demo, this should be enough to see that the plain Sheet has a background.

@hbjORbj
Copy link
Member Author

hbjORbj commented Jun 4, 2022

Added "bg": true to the demo, this should be enough to see that the plain Sheet has a background.

I agree!

@siriwatknp Should we add some other examples other than the playground? Do you have ideas?

@siriwatknp
Copy link
Member

Added "bg": true to the demo, this should be enough to see that the plain Sheet has a background.

I agree!

@siriwatknp Should we add some other examples other than the playground? Do you have ideas?

May be not in this PR since the variant override feature is not done.

@siriwatknp
Copy link
Member

@hbjORbj I wonder if we remove the elevation prop from the sheet. I afraid that developers might be confused because the behavior of the elevation in Joy is different from Material-UI.

We should have lean API as much as possible. The shadow is can be added by the sx prop.

If you are agree with this, please remove the prop from the implementation.

@danilo-leal
Copy link
Contributor

What do you think about changing the default variant value to either outlined / soft / solid? plain doesn't seem to showcase the component well enough:

Hm... I'm not sure about it, I don't think that the point of the default value should be to showcase the component well enough but rather what would be the most common use case for the component in real apps. As the Sheet is a generic container/wrapper component, I'd bet that in most cases, using it plainly, without any borders or lighter backgrounds, would be the most common. But of course, this is my position now, we could change as we get different experiences :)

@hbjORbj
Copy link
Member Author

hbjORbj commented Jun 15, 2022

@danilo-leal Gottcha gottcha 👍
@siriwatknp I removed the elevation prop! :)

@@ -0,0 +1,244 @@
import * as React from 'react';
Copy link
Member

Choose a reason for hiding this comment

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

I created a reusable component for showing the usage playground. Please take a look at ChipUsage.js or SliderUsage.js for how to use.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, done!

Copy link
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

👍 Looks great. left one comment to add color prop to the demo because the description mention the color prop but I don't see it on the demo (I got confused at first).

The `Sheet` component, in addition to the variants, also has access to the `color` prop, allowing you to use every palette of the theme.

docs/data/joy/components/sheet/SimpleSheet.js Outdated Show resolved Hide resolved
docs/data/joy/components/sheet/SimpleSheet.tsx Outdated Show resolved Hide resolved
docs/data/joy/components/sheet/SimpleSheet.tsx.preview Outdated Show resolved Hide resolved
danilo-leal and others added 3 commits June 22, 2022 07:45
Co-authored-by: Siriwat K <siriwatkunaporn@gmail.com>
Co-authored-by: Siriwat K <siriwatkunaporn@gmail.com>
Co-authored-by: Siriwat K <siriwatkunaporn@gmail.com>
@danilo-leal
Copy link
Contributor

danilo-leal commented Jun 22, 2022

Merging this one for now! It's good enough for a first pass, nice work you two!
Further iterations can be handled in #33158.

@danilo-leal danilo-leal merged commit e039a59 into mui:master Jun 22, 2022
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: joy-ui Specific to @mui/joy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants