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 the documentation of the dark theme #19122

Merged
merged 13 commits into from
Jan 13, 2020

Conversation

m4theushw
Copy link
Member

@m4theushw m4theushw commented Jan 7, 2020

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

@oliviertassinari oliviertassinari added the docs Improvements or additions to the documentation label Jan 7, 2020
@mui-pr-bot
Copy link

mui-pr-bot commented Jan 7, 2020

Details of bundle changes.

Comparing: e5b2e22...313505b

bundle Size Change Size Gzip Change Gzip
docs.main ▼ -2 B (-0.00% ) 615 kB ▼ -3 B (-0.00% ) 196 kB
@material-ui/core -- 359 kB -- 98.6 kB
@material-ui/core[umd] -- 314 kB -- 90.8 kB
@material-ui/lab -- 184 kB -- 55 kB
@material-ui/styles -- 51.3 kB -- 15.5 kB
@material-ui/system -- 14.5 kB -- 4.05 kB
Alert -- 84 kB -- 26.4 kB
AlertTitle -- 64.4 kB -- 20.4 kB
AppBar -- 64.2 kB -- 20.2 kB
Autocomplete -- 131 kB -- 41.2 kB
Avatar -- 65.5 kB -- 20.7 kB
AvatarGroup -- 62.5 kB -- 19.7 kB
Backdrop -- 68 kB -- 21.1 kB
Badge -- 65.6 kB -- 20.5 kB
BottomNavigation -- 62.7 kB -- 19.7 kB
BottomNavigationAction -- 75.7 kB -- 24 kB
Box -- 71 kB -- 21.7 kB
Breadcrumbs -- 68.2 kB -- 21.5 kB
Button -- 79.9 kB -- 24.6 kB
ButtonBase -- 74.2 kB -- 23.4 kB
ButtonGroup -- 83.4 kB -- 25.6 kB
Card -- 63.1 kB -- 19.8 kB
CardActionArea -- 75.3 kB -- 23.8 kB
CardActions -- 62.3 kB -- 19.6 kB
CardContent -- 62.2 kB -- 19.6 kB
CardHeader -- 65.3 kB -- 20.6 kB
CardMedia -- 62.6 kB -- 19.8 kB
Checkbox -- 83.2 kB -- 26.4 kB
Chip -- 82.8 kB -- 25.5 kB
CircularProgress -- 64.4 kB -- 20.4 kB
ClickAwayListener -- 3.85 kB -- 1.54 kB
Collapse -- 68.1 kB -- 21.2 kB
colorManipulator -- 3.85 kB -- 1.52 kB
Container -- 63.5 kB -- 19.9 kB
CssBaseline -- 57.8 kB -- 18.2 kB
Dialog -- 83 kB -- 26 kB
DialogActions -- 62.3 kB -- 19.6 kB
DialogContent -- 62.5 kB -- 19.7 kB
DialogContentText -- 64.3 kB -- 20.3 kB
DialogTitle -- 64.5 kB -- 20.3 kB
Divider -- 62.8 kB -- 19.8 kB
docs.landing -- 50.9 kB -- 13.5 kB
Drawer -- 84.7 kB -- 25.9 kB
ExpansionPanel -- 72.4 kB -- 22.8 kB
ExpansionPanelActions -- 62.3 kB -- 19.6 kB
ExpansionPanelDetails -- 62.2 kB -- 19.6 kB
ExpansionPanelSummary -- 78.3 kB -- 24.8 kB
Fab -- 77 kB -- 24.1 kB
Fade -- 23.3 kB -- 7.99 kB
FilledInput -- 73.8 kB -- 23 kB
FormControl -- 64.6 kB -- 20.2 kB
FormControlLabel -- 65.8 kB -- 20.7 kB
FormGroup -- 62.3 kB -- 19.6 kB
FormHelperText -- 63.5 kB -- 20 kB
FormLabel -- 63.7 kB -- 19.8 kB
Grid -- 65.3 kB -- 20.6 kB
GridList -- 62.7 kB -- 19.8 kB
GridListTile -- 64 kB -- 20.1 kB
GridListTileBar -- 63.5 kB -- 20 kB
Grow -- 23.9 kB -- 8.21 kB
Hidden -- 66.2 kB -- 20.8 kB
Icon -- 63 kB -- 19.8 kB
IconButton -- 76.4 kB -- 23.9 kB
Input -- 72.7 kB -- 22.8 kB
InputAdornment -- 65.3 kB -- 20.6 kB
InputBase -- 70.8 kB -- 22.3 kB
InputLabel -- 65.6 kB -- 20.3 kB
LinearProgress -- 65.6 kB -- 20.5 kB
Link -- 66.8 kB -- 21.2 kB
List -- 62.6 kB -- 19.6 kB
ListItem -- 77.3 kB -- 24.3 kB
ListItemAvatar -- 62.4 kB -- 19.6 kB
ListItemIcon -- 62.4 kB -- 19.6 kB
ListItemSecondaryAction -- 62.3 kB -- 19.6 kB
ListItemText -- 65.2 kB -- 20.6 kB
ListSubheader -- 63 kB -- 19.9 kB
Menu -- 88.7 kB -- 27.4 kB
MenuItem -- 78.4 kB -- 24.6 kB
MenuList -- 66.2 kB -- 20.8 kB
MobileStepper -- 68.1 kB -- 21.4 kB
Modal -- 14.3 kB -- 5.03 kB
NativeSelect -- 77 kB -- 24.4 kB
NoSsr -- 2.19 kB -- 1.04 kB
OutlinedInput -- 74.2 kB -- 23.2 kB
Paper -- 62.6 kB -- 19.6 kB
Popover -- 83 kB -- 25.8 kB
Popper -- 28.7 kB -- 10.3 kB
Portal -- 2.9 kB -- 1.3 kB
Radio -- 84.3 kB -- 26.7 kB
RadioGroup -- 64.7 kB -- 20.1 kB
Rating -- 70.7 kB -- 22.8 kB
RootRef -- 4.21 kB -- 1.64 kB
Select -- 116 kB -- 34.5 kB
Skeleton -- 63.2 kB -- 20.1 kB
Slide -- 25.3 kB -- 8.72 kB
Slider -- 76.8 kB -- 24.4 kB
Snackbar -- 75.4 kB -- 23.7 kB
SnackbarContent -- 63.8 kB -- 20.2 kB
SpeedDial -- 86.3 kB -- 27.3 kB
SpeedDialAction -- 118 kB -- 37.5 kB
SpeedDialIcon -- 64.8 kB -- 20.4 kB
Step -- 62.9 kB -- 19.8 kB
StepButton -- 82.5 kB -- 26.2 kB
StepConnector -- 63 kB -- 19.9 kB
StepContent -- 69.2 kB -- 21.8 kB
StepIcon -- 64.9 kB -- 20.3 kB
StepLabel -- 68.8 kB -- 21.7 kB
Stepper -- 65.1 kB -- 20.6 kB
styles/createMuiTheme -- 16.5 kB -- 5.85 kB
SvgIcon -- 63.3 kB -- 19.8 kB
SwipeableDrawer -- 92.1 kB -- 29 kB
Switch -- 82.4 kB -- 26.1 kB
Tab -- 76.6 kB -- 24.3 kB
Table -- 62.8 kB -- 19.8 kB
TableBody -- 62.4 kB -- 19.6 kB
TableCell -- 64.3 kB -- 20.3 kB
TableContainer -- 62.2 kB -- 19.6 kB
TableFooter -- 62.4 kB -- 19.6 kB
TableHead -- 62.4 kB -- 19.6 kB
TablePagination -- 142 kB -- 41.8 kB
TableRow -- 62.8 kB -- 19.8 kB
TableSortLabel -- 77.6 kB -- 24.5 kB
Tabs -- 85.7 kB -- 27.3 kB
TextareaAutosize -- 5.09 kB -- 2.14 kB
TextField -- 125 kB -- 36.5 kB
ToggleButton -- 76.4 kB -- 24.3 kB
ToggleButtonGroup -- 63.5 kB -- 20 kB
Toolbar -- 62.6 kB -- 19.7 kB
Tooltip -- 102 kB -- 32.4 kB
TreeItem -- 74 kB -- 23.5 kB
TreeView -- 66.9 kB -- 21.1 kB
Typography -- 63.9 kB -- 20.1 kB
useAutocomplete -- 14.3 kB -- 5.18 kB
useMediaQuery -- 2.5 kB -- 1.05 kB
Zoom -- 23.4 kB -- 8.11 kB

Generated by 🚫 dangerJS against 313505b

@mbrookes
Copy link
Member

mbrookes commented Jan 8, 2020

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:

> Tip: you can play with the documentation theme object in your browser console,
as the `theme` variable is exposed on all the documentation pages.
Please note that **the documentation site is using a custom theme**.

Tip: you can play with the documentation theme object in your browser console,
as the theme variable is exposed on all the documentation pages.
Please note that the documentation site is using a custom theme.

@m4theushw
Copy link
Member Author

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.

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?

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.)

I don't think so. Only the palette key gets affected by dark mode.

Regarding the page https://material-ui.com/customization/palette#dark-mode, it would be nice to improve the demo adding Material-UI components (like Paper and Typography) using the affected colors. What do you think?

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 8, 2020

I'm wondering if we should have a control in the demo to toggle dark mode

I think that we should prefer using the documentation global theme switches. We could also add the RTL and color changes, for simplicity.

Would this be a good place to document what colors are affected by dark mode?

I'm not sure we need to change the markdown for it, I would suspect reading the source be more valuable.

Perhaps in v5 we should update the default colours to match the documentation?

Maybe we can do better, yeah :)

@mbrookes
Copy link
Member

mbrookes commented Jan 8, 2020

I chose to use the docs toggle to not confuse users with two toggles.

On the other hand, it reinforces the recurring confusion that the inspector is show the docs theme, and that this is the default.

Should I change to provide a control inside the inspector?

I think so.

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.)

Only the palette key gets affected by dark mode.

Yes, this is what we had previously documented:

https://v4-7-2.material-ui.com/customization/palette/#type-light-dark-theme

@mbrookes
Copy link
Member

mbrookes commented Jan 8, 2020

I think that we should prefer using the documentation global theme switches.

We aren't displaying the documentation theme (the description is incorrect).

@oliviertassinari
Copy link
Member

Make sense, I guess both are good enough :)

@mbrookes
Copy link
Member

mbrookes commented Jan 8, 2020

Cool. If in the future we bring the docs and default theme into alignment we can revisit the controls.

@mbrookes
Copy link
Member

mbrookes commented Jan 8, 2020

Not directly related to this PR, but I noticed the dark theme has palette.text.icon. @oliviertassinari is that legacy and can it be removed? (It isn't used in any components.)

@m4theushw
Copy link
Member Author

@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

@mbrookes
Copy link
Member

mbrookes commented Jan 8, 2020

@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? (hideHeader: true)

@oliviertassinari
Copy link
Member

is that legacy and can it be removed? (It isn't used in any components.)

@mbrookes Great catch, yes, I think that we should remove it in v5. @m4theushw Could you remove it from the documentation (x2)?

Also, should we disable the "header" on these two? (hideHeader: true)

In this case, we can hide the header for Intention.js too. I would also suggest setting "bg": "inline" to remove the paper.


A couple more feedback

  • I'm not sure we need to mention "dark theme" or "light theme" in the demo.
  • I'm wondering about the unification we could find with the demo in #intentions.
  • Maybe we could use some groups text -> Typography, action -> Buttons, divider -> Divider.
  • I find it interesting to display a more complete theme path (palette.text.secondary). It's less room for guessing.

@m4theushw
Copy link
Member Author

All suggestions applied. @oliviertassinari I didn't find any mention of palette.text.icon in docs.

Copy link
Member

@joshwooding joshwooding left a 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.

docs/src/pages/customization/palette/palette.md Outdated Show resolved Hide resolved
@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 9, 2020

I didn't find any mention of palette.text.icon in docs.

@m4theushw Oops, sorry I think that. Matt and I meant palette.text.hint.

@mbrookes
Copy link
Member

mbrookes commented Jan 9, 2020

I noticed the dark theme has palette.text.icon. @oliviertassinari is that legacy and can it be removed? (It isn't used in any components.)

I didn't find any mention of palette.text.icon in docs.

@m4theushw Oops, sorry I think that. Matt and I meant palette.text.hint.

No, I meant palette.text.icon:

image

(Although I notice that palette.text.hint is the same in dark and light if that's what @oliviertassinari was referring to?)

docs/src/modules/components/MarkdownElement.js Outdated Show resolved Hide resolved
@@ -4,13 +4,13 @@

## Explore

Explore the documentation theme object:
Explore the default theme object:
Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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?

Copy link
Member

@oliviertassinari oliviertassinari Jan 11, 2020

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?

Copy link
Member

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.

Copy link
Member

Choose a reason for hiding this comment

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

Sure

@m4theushw
Copy link
Member Author

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/

@oliviertassinari
Copy link
Member

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.

@m4theushw
Copy link
Member Author

I think we are having some misunderstanding here. :)

@mbrookes @oliviertassinari

No, I meant palette.text.icon:

palette.text.icon is not mentioned in docs, it just have to be removed from createPalette.js in v5.

While that palette.text.hint is not used in any component too, but it's documented.

Should I revert 7ac20de?

@oliviertassinari
Copy link
Member

@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 🌱.

@oliviertassinari oliviertassinari changed the title [docs] Make inspector aware of current theme [docs] Improve the documentation of the dark theme Jan 10, 2020
@oliviertassinari oliviertassinari merged commit 5633c27 into mui:master Jan 13, 2020
@oliviertassinari
Copy link
Member

@m4theushw Well done!

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Documentation] Better documentation of light/dark theme transition
5 participants