-
-
Notifications
You must be signed in to change notification settings - Fork 31.6k
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][material] Improve documentation about adding custom colors #37743
[docs][material] Improve documentation about adding custom colors #37743
Conversation
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.
Great addition @DiegoAndai ! My comments here are mostly about style and structure.
Co-authored-by: Sam Sycamore <71297412+samuelsycamore@users.noreply.github.com> Signed-off-by: Diego Andai <diego@mui.com>
Co-authored-by: Sam Sycamore <71297412+samuelsycamore@users.noreply.github.com> Signed-off-by: Diego Andai <diego@mui.com>
Co-authored-by: Sam Sycamore <71297412+samuelsycamore@users.noreply.github.com> Signed-off-by: Diego Andai <diego@mui.com>
Co-authored-by: Sam Sycamore <71297412+samuelsycamore@users.noreply.github.com> Signed-off-by: Diego Andai <diego@mui.com>
After @samuelsycamore's feedback, I started reworking some examples and concluded that the palette docs needed a reorganization. Related information was scattered and the structure wasn't easy-to-follow. Here are the changes I introduced:
I was very cautious of not removing anything, but rather reorganizing. Everything that was there is still there, except for the non-palette colors explanation as that was redirected to custom variables. Notes
|
@DiegoAndai The demos look great! I noticed that the text lists the colors in the order |
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.
Sorry it took me so long to get to this @DiegoAndai ! This is a major improvement for sure. The vast majority of my suggestions here are for copy editing, style, and grammar. We should make sure that the hierarchy of headers and subheaders makes sense in the final draft.
Co-authored-by: Sam Sycamore <71297412+samuelsycamore@users.noreply.github.com> Signed-off-by: Diego Andai <diego@mui.com>
Co-authored-by: Sam Sycamore <71297412+samuelsycamore@users.noreply.github.com> Signed-off-by: Diego Andai <diego@mui.com>
Co-authored-by: Sam Sycamore <71297412+samuelsycamore@users.noreply.github.com> Signed-off-by: Diego Andai <diego@mui.com>
Co-authored-by: Sam Sycamore <71297412+samuelsycamore@users.noreply.github.com> Signed-off-by: Diego Andai <diego@mui.com>
Co-authored-by: Sam Sycamore <71297412+samuelsycamore@users.noreply.github.com> Signed-off-by: Diego Andai <diego@mui.com>
Co-authored-by: Sam Sycamore <71297412+samuelsycamore@users.noreply.github.com> Signed-off-by: Diego Andai <diego@mui.com>
Co-authored-by: Sam Sycamore <71297412+samuelsycamore@users.noreply.github.com> Signed-off-by: Diego Andai <diego@mui.com>
Co-authored-by: Sam Sycamore <71297412+samuelsycamore@users.noreply.github.com> Signed-off-by: Diego Andai <diego@mui.com>
Hey @samuelsycamore and @mj12albert! thanks for the feedback, I implemented it and it's ready for your re-review @samuelsycamore regarding this comment, maybe we could just list the colors and then link to Material's Color System for the explanations? |
That sounds good to me. Go for it! 🤝 |
@samuelsycamore I reworded the color's descriptions to be shorter and added a link to Material's Color System, let me know what 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.
Looks great!
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! 🚀
Closes: #29595
Summary
Improve documentation regarding adding new colors using a color object:
light
,dark
, andcontrastText
tokens are required and not calculated automatically, which differs from default colors.augmentColor
function to augment custom colors.Note
I also wanted to improve the warning/error that users would get when using a component that requires
light
,dark
, orcontrastText
(see theChip
'scontrastText
issue: #37676). But after a second thought, it seemed very cumbersome and overhead to do given the small use case for which the warning would be helpful.If you disagree and think it would be worth it, I would love to discuss approaches to adding the warning 😊 cc: @mui/core