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

[material-ui][docs] Add improvements to Dashboard template #42445

Open
wants to merge 63 commits into
base: next
Choose a base branch
from

Conversation

zanivan
Copy link
Contributor

@zanivan zanivan commented May 29, 2024

Closes #42610
Preview: https://deploy-preview-42445--material-ui.netlify.app/material-ui/getting-started/templates/dashboard/

Objective

👉 This is a further PR to add small tweaks and fixes to the Dashboard template after #41757.


Tasks

Adding some tweaks to the template based on Olivier's feedback #42610

  • 1. Fix scrollbar origin
scrollbar
  • 2. Fix the dark mode toggle to persist between loads (will be addressed in a future PR)
Screen.Recording.2024-06-01.at.20.32.43.mov
  • 3. Fix keyboard focus style on the tabs Fixed since we removed tabs
overflow tab
  • 4. Polish out the table design (still missing some tweaks on the menu, and spacing)
SCR-20240601-rxxo

#41757 (comment)

  • 5. Add side navigation menu (add mobile version)

@zanivan zanivan added docs Improvements or additions to the documentation package: material-ui Specific to @mui/material design This is about UI or UX design, please involve a designer labels May 29, 2024
@zanivan zanivan self-assigned this May 29, 2024
@mui-bot
Copy link

mui-bot commented May 29, 2024

Netlify deploy preview

https://deploy-preview-42445--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 6eb9198

Copy link
Contributor

@danilo-leal danilo-leal left a comment

Choose a reason for hiding this comment

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

Awesome — dropping a first pass here.

@zanivan
Copy link
Contributor Author

zanivan commented May 29, 2024

@noraleonte I noticed that the TreeView has a custom class for focus, and I couldn't disable it on click. I believe the focus-visible styles are showing up here because this class acts in place of the native classes, right? What is the difference/benefit of this custom class versus the native one? 🤔

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 5, 2024
Copy link
Contributor

@noraleonte noraleonte left a comment

Choose a reason for hiding this comment

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

Great work on these improvement 🎉 I am leaving some nitpicks to finetune, let me know if you need help with any of them. 🎉

@zanivan zanivan marked this pull request as draft June 5, 2024 13:36
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 5, 2024
@zanivan
Copy link
Contributor Author

zanivan commented Jun 6, 2024

Hey @noraleonte, thanks for the feedback! I'm still adding many changes, but I'll definitely need some help code-wise. Can you help me with these?

1. Making menus consistent:

All these menus should look similar regarding states and padding. The 'Development' select is IMO the best, so if others could be like that would be amazing!

Screenshot 2024-06-06 at 14 34 20 Screenshot 2024-06-06 at 14 34 10 Screenshot 2024-06-06 at 14 34 32

2. Some of Olivier's feedbacks:

  1. Fix scrollbar origin
  2. Fix the dark mode toggle to persist between loads
  3. Fix keyboard focus style on the tabs
  4. Make the data grid keyboard focus scroll the page

3. Performance and best practices

This should be the reference of best practices regarding customising Material UI and X, so it's important to review everything and reassure that:

  • Users can learn how to customize components from this;
  • Users can easily copy chunks of code, regardless the theme is selected;
  • No third-party code from docs or other sources that the users won't know of;
  • Be accessible and score as high as possible on Lighthouse score;

I tried to cover all of these, but someone with a better code knowledge would likely spot some loose ends 👍

Copy link
Member

@DiegoAndai DiegoAndai left a comment

Choose a reason for hiding this comment

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

@zanivan here is some feedback, I haven't gone through the entire PR yet

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 1, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 1, 2024
@zanivan zanivan requested a review from DiegoAndai July 1, 2024 20:19
sx={{
maxHeight: 56,
width: 215,
'&.MuiList-root': {
Copy link
Member

Choose a reason for hiding this comment

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

We should use listClasses.root

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried replacing it for:

        [`&. ${listClasses.root}`]: {
          p: '8px',
        },

and importing it like:
import listClasses from '@mui/material/List';

However, I got the error:
Property 'root' does not exist on type 'ExtendList<ListTypeMap<{}, "ul">>'.ts(2339)

@@ -10,6 +10,21 @@ import Typography from '@mui/material/Typography';
import { SparkLineChart } from '@mui/x-charts/SparkLineChart';
import { areaElementClasses } from '@mui/x-charts/LineChart';

function getDaysInMonth(month, year) {
Copy link
Member

Choose a reason for hiding this comment

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

The docs package has date-fns as a dependency, could we use it for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to clarify, is it a dependency for MUI/docs or for the Material UI package as well? If it's not a dependency for Material UI, I think we shouldn't include it in the templates, since we don't want to bundle extra stuff in the templates other than Material UI's dependencies.

'&:hover': {
backgroundColor: theme.palette.action.hover,
},
'&.Mui-selected': {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DiegoAndai couldn't find an alternative for replacing this one

Copy link
Contributor

Choose a reason for hiding this comment

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

I was also looking for an import for this class recently (commenting to get GH updates on this 👀)

boxShadow:
'hsla(220, 30%, 5%, 0.07) 0px 4px 16px 0px, hsla(220, 25%, 10%, 0.07) 0px 8px 16px -5px',
[`& .${buttonBaseClasses.root}`]: {
'&.Mui-selected': {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above

backgroundColor: theme.palette.background.paper,
boxShadow: 'none',
},
[`&.${selectClasses.focused}`]: {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm getting an error here Property 'focused' does not exist on type 'SelectClasses'.ts(2339), however, it seems that selectClasses does have a focused property 🤔

Copy link
Contributor

@KenanYusuf KenanYusuf left a comment

Choose a reason for hiding this comment

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

Looking great. There are a few minor things I noticed:

1. The "Rows per page" select is missing a focus style
rows-per-page-focus

2. On the tree view, focusing on the parent elements also adds a focus style to the child elements.

product-tree.mp4

3. On the data grid, the horizontal spacing could be tightened slightly in the menu overlays. 4px left/right and 8px top/bottom?

Screenshot 2024-07-08 at 10 11 44

4. The alignment looks off between the checkboxes and search field.

Screenshot 2024-07-08 at 10 14 22

5. The select fields in the filter panel need some horizontal spacing, and the text input could be updated to be more in line with the select styles.

Screenshot 2024-07-08 at 10 15 02

6. The data grid could use an overlay height to prevent it collapsing like this when filters are applied:

Screenshot 2024-07-08 at 10 23 46

7. There is a bit of a visual glitch when grid menus open. The box-shadow is fully visible whilst the menu is animating in and out.

data-grid-shadow.mp4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design This is about UI or UX design, please involve a designer docs Improvements or additions to the documentation package: material-ui Specific to @mui/material
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[material-ui] Dashboard template improvements
7 participants