-
-
Notifications
You must be signed in to change notification settings - Fork 32.5k
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] Revise and expand Joy UI "Alert" page #34838
[docs] Revise and expand Joy UI "Alert" page #34838
Conversation
|
I want to tag @danilo-leal for a review but for some reason I'm not able to. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hah, the tagging thing is because of a dumb GitHub access issue I had, nevermind it! 😅
This is looking amazing, though⎯ways better than it was before, particularly the Accessibility section, nice work in there!
<Alert component="span" /> | ||
``` | ||
|
||
## Customization |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
## Customization | |
## Props |
Customization is quite broad and for Joy UI, maybe it is lean toward styling.
Using Props could be more direct. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The goal with adding this header is to try to match the formatting of the Base docs, so we'll have consistency across the products. I think it's better to keep this header more broad, because not everything that would fall under this category is about props—for example, the Avatar doc describes a few ways to customize using CSS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm with Sam on this one because I think that almost all the API of the components are already props.
so we'll have consistency across the products
I think we should first check it it's compatible with Material UI because Joy UI is closer to it than MUI Base. At first glance, I would expect the Joy UI and Material UI docs heading structure to be very close, but it's not clear to me that it would work with MUI Base.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the structure that I'm using for the updated Joy docs will work well enough for Material UI going forward. It's very close to what I created for the Base docs but with some minor adjustments to better suit the product, and I think the same thing will happen when I eventually move on from Joy to Material UI.
Here's a related question: Many of these component pages that are in both Joy & Material UI docs contain shared/copied content. It would be kind of silly for me to come up with new ways to paraphrase these descriptions for each product, so it makes sense that some of the content would be repeated. With that in mind, maybe I should revise the Joy and Material UI pages at the same time (wherever they share components)? It would take a bit longer to complete the project on the Joy UI side, but it would get the ball rolling for Material UI component page revisions. And then we could confirm that the updated structure works for both products, or else adjust as needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for fun 😅 I revised the Material UI Alert page to see if it makes sense to work on these pages together. #34892
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@samuelsycamore For the component prop, I agree, no need to mention the slots if there is only a root on the page that you modified, it's a guide, not a reference page.
Regarding the page itself, I think that there is too much to scroll to find one live demo, something they can trust they can copy & paste and will work. I would recommend starting with one very simple demo. For example, we could say that Basics, https://deploy-preview-34838--material-ui.netlify.app/joy-ui/react-alert/#basics in our case, should always be a live demo:
@oliviertassinari that's a great idea! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 LGTM, left one suggestion about the default variant (should be soft
, not `solid)
Co-authored-by: Siriwat K <siriwatkunaporn@gmail.com> Signed-off-by: Sam Sycamore <71297412+samuelsycamore@users.noreply.github.com>
Co-authored-by: Sam Sycamore <71297412+samuelsycamore@users.noreply.github.com> Signed-off-by: danilo leal <67129314+danilo-leal@users.noreply.github.com>
Part of #33998
This PR revises and expands the Joy UI "Alert" page.
My primary concern here was to replace copy+paste text from the ARIA APG (which also needs to happen for the Material UI Alert page). I also wanted to add some clarity on usage.
Structurally, I tried to bring it closer to the standard that we settled on in the Base docs. One thing I've done differently here is in how the slot props are presented. In the Base docs, every component page lists all props (
component
,slots
,slotProps
), even when the latter two are redundant (when the component only has a root slot). In this doc I've only called out thecomponent
prop, since it's the only one you'd actually use with this component. I think this is preferable. @oliviertassinari I'm curious what you think about this.I also expanded on the Accessibility section, adding more info that I found through ARIA APG and WCAG.
https://deploy-preview-34838--material-ui.netlify.app/joy-ui/react-alert/