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] Add density guide to customizations #16410

Merged
merged 25 commits into from
Jul 9, 2019

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Jun 28, 2019

Addresses #14521

This is me loosely playing around with https://material.io/design/layout/applying-density.html#

I think the final implementation will somewhat look like this:

  • createMuiTheme({ dense: true }) will add the flag to the resulting theme object which is used by the components if the props regarding density aren't set. Theme density is overridden by applying density via context (e.g. list, table) or props.
  • docs get a guide under /customization/themes/density which will include a switch to apply it to the website
  • we document our density theme: I think dense themes can't apply universally (screen targets, device targets etc) which will likely result in a theme that targets a wider audience than most use cases. For instance admin dashboards for desktop don't need to consider touch hit targets while we should for showcasing purposes.

@fzaninotto might be interested in this one. How are you currently applying density? How would you like the core API to look like?

@eps1lon eps1lon added discussion package: material-ui Specific to @mui/material labels Jun 28, 2019
@mui-pr-bot
Copy link

mui-pr-bot commented Jun 28, 2019

Details of bundle changes.

Comparing: 7d826a9...90fdb4f

bundle parsed diff gzip diff prev parsed current parsed prev gzip current gzip
@material-ui/core 0.00% 0.00% 327,349 327,349 90,365 90,365
@material-ui/core/Paper 0.00% 0.00% 68,290 68,290 20,375 20,375
@material-ui/core/Paper.esm 0.00% 0.00% 61,574 61,574 19,155 19,155
@material-ui/core/Popper 0.00% 0.00% 28,942 28,942 10,408 10,408
@material-ui/core/Textarea 0.00% 0.00% 5,505 5,505 2,365 2,365
@material-ui/core/TrapFocus 0.00% 0.00% 3,753 3,753 1,576 1,576
@material-ui/core/styles/createMuiTheme 0.00% 0.00% 16,012 16,012 5,793 5,793
@material-ui/core/useMediaQuery 0.00% 0.00% 2,595 2,595 1,103 1,103
@material-ui/lab 0.00% 0.00% 137,896 137,896 42,523 42,523
@material-ui/styles 0.00% 0.00% 51,699 51,699 15,348 15,348
@material-ui/system 0.00% 0.00% 15,428 15,428 4,395 4,395
Button 0.00% 0.00% 84,442 84,442 25,738 25,738
Modal 0.00% 0.00% 14,515 14,515 5,088 5,088
Portal 0.00% 0.00% 3,471 3,471 1,573 1,573
Slider 0.00% 0.00% 74,976 74,976 23,292 23,292
colorManipulator 0.00% 0.00% 3,904 3,904 1,543 1,543
docs.landing 0.00% 0.00% 54,338 54,338 13,762 13,762
docs.main +0.14% 🔺 +0.14% 🔺 647,009 647,912 203,904 204,192
packages/material-ui/build/umd/material-ui.production.min.js 0.00% 0.00% 299,721 299,721 85,971 85,971

Generated by 🚫 dangerJS against 90fdb4f

@fzaninotto
Copy link
Contributor

Super interesting indeed. We're forcing high density because we're essentially a desktop framework. In an ideal world, the density would change automatically depending on the screen size. I wouldn't even give the ability to the user to change that default as it's the best for both words (high density on Desktop, larger touch targets on Mobile).

@eps1lon
Copy link
Member Author

eps1lon commented Jun 28, 2019

I wouldn't even give the ability to the user to change that default as it's the best for both words (high density on Desktop, larger touch targets on Mobile).

And then enter the world of desktop touch displays. I would prefer to leave this to theme authors since high density isn't always useful for desktop. I'd even argue most of the time you don't want high density (apart from dashboards with a high information density).

So customization is still a must have for us. I was more thinking about whether this is useful as a single flag or if you apply this only to certain views in your UI. But it sounds like you have high density everywhere.

@eps1lon eps1lon force-pushed the feat/theme-presets branch from feb47b9 to 7ed5465 Compare July 4, 2019 12:32
@eps1lon eps1lon marked this pull request as ready for review July 5, 2019 09:36
@eps1lon eps1lon force-pushed the feat/theme-presets branch from 4bb57b4 to 534251f Compare July 5, 2019 09:37
@eps1lon eps1lon requested a review from mbrookes July 5, 2019 10:16
@mbrookes
Copy link
Member

mbrookes commented Jul 5, 2019

@eps1lon "I was more thinking about whether this is useful as a single flag or if you apply this only to certain views in your UI."

That was my initial thought too, when looking at this PR vs. what the spec says:

When to apply density
Components with high density enable users to process and take action against large amounts of information in a more manageable way."

When not to apply density
Don’t apply density to components that alert the user of changes, such as snackbars or dialogs. Applying high density to alerts reduces their ability to command attention.

For example, globally applying density would give dense (small) buttons to the dialog (one of the "don'ts" in the accompanying image.

However, since we let devs deviate from the Material spec in every other way, I don't see an issue per-se with making it easier to customise it globally.

It then becomes a question of whether it's useful enough to justify the added complexity, and small bundle-size increase.

docs/src/pages/customization/density/density.md Outdated Show resolved Hide resolved
docs/src/pages/customization/density/density.md Outdated Show resolved Hide resolved
docs/src/pages/customization/density/density.md Outdated Show resolved Hide resolved
@eps1lon
Copy link
Member Author

eps1lon commented Jul 6, 2019

"I was more thinking about whether this is useful as a single flag or if you apply this only to certain views in your UI."

It's definitely an easy flag to abuse. Maybe add an additional warning that we encourage this option only for certain views (e.g. dashboard) and not your whole application. And then encourage to actually read the guidelines and think about implications.

@oliviertassinari How did you envision #14521 to be resolved?

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 6, 2019

How did you envision #14521 to be resolved?

@eps1lon I was thinking of adding the size=small prop to all the components that need it. From what I remember, we were missing one case for the filled input. I had nothing plan for the theme object.

@eps1lon
Copy link
Member Author

eps1lon commented Jul 6, 2019

FilledInput already has margin="dense".

So we cut it from the theme and just list the components that have some properties that change densitiy?

@oliviertassinari
Copy link
Member

FilledInput already has margin="dense".

I'm on my phone. From what I remember, the filled input has two different dense modes. I believe we only support one.

Regarding the theme global prop switch. The idea has potential, it makes me think of the Gmail dense feature. I would be cautious to make sure it's a feature our users want. For instance, the menu has a dense prop that seems to be for mobile devices, should it be activated on the dense desktop? I'm not sure. Maybe the a la carte approach is safer?

@eps1lon
Copy link
Member Author

eps1lon commented Jul 6, 2019

Maybe the a la carte approach is safer?

No objection here. It's what I would recommend anyway since you have to consider the devices you target and where this gets applied (see matts example on dialogs).

So do we go back to fee8986 for our docs and keep the new page with the density tool? We should have some entry point for density in our docs.

@oliviertassinari
Copy link
Member

👍 from my end to start with the existing theme component default prop API and to grow from here.

@eps1lon eps1lon changed the title [core] Add global density option to theme [docs] Add density guide to customizations Jul 8, 2019
@eps1lon eps1lon added docs Improvements or additions to the documentation and removed discussion package: material-ui Specific to @mui/material labels Jul 8, 2019
@eps1lon eps1lon force-pushed the feat/theme-presets branch from 0bdd728 to 4abb4f2 Compare July 8, 2019 08:42
@eps1lon eps1lon force-pushed the feat/theme-presets branch from 4abb4f2 to b9657d6 Compare July 8, 2019 08:43
Co-Authored-By: Matt <github@nospam.33m.co>
@eps1lon eps1lon requested a review from mbrookes July 8, 2019 09:54
docs/src/pages/customization/density/density.md Outdated Show resolved Hide resolved
docs/src/pages/customization/density/density.md Outdated Show resolved Hide resolved
docs/src/pages/customization/density/density.md Outdated Show resolved Hide resolved
Co-Authored-By: Matt <github@nospam.33m.co>
@eps1lon eps1lon merged commit 485916a into mui:master Jul 9, 2019
@eps1lon eps1lon deleted the feat/theme-presets branch July 9, 2019 07:37
@oliviertassinari
Copy link
Member

Really nice. Should it closes #14521?

@eps1lon
Copy link
Member Author

eps1lon commented Jul 9, 2019

Really nice. Should it closes #14521?

You said earlier that it should be closed by adding the missing filled density variant which is implemented in #16515

@oliviertassinari
Copy link
Member

Oh, my bad, thanks for the update! I thought we had two issues open for the dense support. We don't.

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.

5 participants