-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
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 the documentation of the dark theme #19122
Conversation
Details of bundle changes.Comparing: e5b2e22...313505b
|
I'm wondering if we should have a control in the demo to toggle dark mode, rather than depending on the docs toggle. It would be clearer, and remove the need for the tip, as well as not requiring to change from the users preferred mode. We should change the description to "Here you can explore the default theme object." ("default", not "documentation"). Would this be a good place to document what colors are affected by dark mode? (This info was recently removed from https://material-ui.com/customization/palette#dark-mode.) Aside: I noticed that the primary colors shown in the palette under Intentions on that page are using the docs colors rather than the default. Perhaps in v5 we should update the default colours to match the documentation? While we're working on this page, I would change the console tip to:
|
I was also thinking about it but I chose to use the docs toggle to not confuse users with two toggles. Should I change to provide a control inside the inspector?
I don't think so. Only the Regarding the page https://material-ui.com/customization/palette#dark-mode, it would be nice to improve the demo adding Material-UI components (like |
I think that we should prefer using the documentation global theme switches. We could also add the RTL and color changes, for simplicity.
I'm not sure we need to change the markdown for it, I would suspect reading the source be more valuable.
Maybe we can do better, yeah :) |
On the other hand, it reinforces the recurring confusion that the inspector is show the docs theme, and that this is the default.
I think so.
Yes, this is what we had previously documented: https://v4-7-2.material-ui.com/customization/palette/#type-light-dark-theme |
We aren't displaying the documentation theme (the description is incorrect). |
Make sense, I guess both are good enough :) |
Cool. If in the future we bring the docs and default theme into alignment we can revisit the controls. |
Not directly related to this PR, but I noticed the dark theme has |
@mbrookes Could you take a look into the Palette page? I made a change to the Dark mode section. https://deploy-preview-19122--material-ui.netlify.com/customization/palette/#dark-mode |
@m4theushw Great! That's much more useful than the current demo. While you're working on it, did you want to adjust the intentions demo to use the default theme? Also, should we disable the "header" on these two? ( |
@mbrookes Great catch, yes, I think that we should remove it in v5. @m4theushw Could you remove it from the documentation (x2)?
In this case, we can hide the header for Intention.js too. I would also suggest setting A couple more feedback
|
All suggestions applied. @oliviertassinari I didn't find any mention of |
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 one thing otherwise looks good.
@m4theushw Oops, sorry I think that. Matt and I meant |
No, I meant (Although I notice that |
@@ -4,13 +4,13 @@ | |||
|
|||
## Explore | |||
|
|||
Explore the documentation theme object: | |||
Explore the default theme object: |
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 have an impression of déjà vu, I wonder if we didn't iterate on this already in the past. If we did, it would be interesting to see the discussion we had.
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 had the same feeling (and had a quick look at git blame to try to find a related PR/issue, without any luck). As it stands, we show the default theme, so that's what we should document in the markdown.
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.
2 years ago: #9857. It seems that we still didn't find a good approach.
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.
Good find. Option 2 from the previous list - aligning the default theme and docs colors is the next move for v5, as already tentatively agreed.
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.
Ok, so we move forward like this?
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.
What about we change the default theme colors in a minor?
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.
It's a relatively low impact change – it won't actually break anything, and if a site isn't customising even the theme basics, it might not matter much to them; but IMHO it would break expectations to change this in a minor – to me, a visually breaking change is still a breaking change.
Combined with the fact that this has been a low priority for a long time without any significant pushback, I suggest that we put it on hold, but with a clear plan to address it in v5.
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.
Sure
The Palette page inside System was also using the docs theme. I took the opportunity to change it to the default theme. https://deploy-preview-19122--material-ui.netlify.com/system/palette/ |
I don't think it will be necessary. I think that we can keep the source simple. I think that the actual values in these system demos don't really matter. |
I think we are having some misunderstanding here. :)
While that Should I revert 7ac20de? |
7e57d32
to
313505b
Compare
@m4theushw I have pushed a few changes to polish it. As far as I'm concerned, we are good :). But It would be great to double-check, it's a good step forward 🌱. |
@m4theushw Well done! |
This PR improves the inspector in the "Default Theme" page by making it show values according to the chosen theme (light / dark). This was requested in #19067.
Closes #19067