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][joy-ui] Refine the Messages template #38807

Merged
merged 14 commits into from
Sep 12, 2023
Merged

Conversation

zanivan
Copy link
Contributor

@zanivan zanivan commented Sep 4, 2023

Refining the Messages template, aiming to remove most of the custom styles and overrides, making it closer as possible to the default theme.

Part of #38582

https://deploy-preview-38807--material-ui.netlify.app/joy-ui/getting-started/templates/messages/

@zanivan zanivan added docs Improvements or additions to the documentation package: joy-ui Specific to @mui/joy design: joy This is about Joy Design, please involve a visual or UX designer in the process labels Sep 4, 2023
@zanivan zanivan self-assigned this Sep 4, 2023
@mui-bot
Copy link

mui-bot commented Sep 4, 2023

Netlify deploy preview

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

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 91e8e87

@zanivan zanivan mentioned this pull request Sep 4, 2023
10 tasks
@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 6, 2023

The new side nav menu looks a lot better 👍

  1. There are multiple issues with the keyboard, the focus style is off, and some of the items can't be tab to.
Screen.Recording.2023-09-06.at.15.16.12.mov
  1. The scroll container position feels off, it should have no gap with the border, no?
Screenshot 2023-09-06 at 15 13 01

@zanivan
Copy link
Contributor Author

zanivan commented Sep 6, 2023

There are multiple issues with the keyboard, the focus style is off, and some of the items can't be tab to.

Hey thanks for the feedback! It's probably due to the usage of accordion component instead of the list on the sidebar. I'll fix this, like @siriwatknp did on #38599.

@siriwatknp
Copy link
Member

@zanivan Please see the fix in 93994c5, the root cause is that, ListItem produce <li> which should not be a descendant of the upper <li>.

Before

<li>
  <div role="button">
    <li></li> {/* this is wrong */}
  </div>
</li>

After

<li>
  <div role="button">
    <div></div>
  </div>
</li>

For nested list, it must follow ul > li > ul > li, … and so on.

@zanivan zanivan marked this pull request as ready for review September 8, 2023 18:20
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.

This is looking great! I'm excited to see more templates getting refined! 🤘
Some things I noticed we could improve:

Screenshot Note
Screen Shot 2023-09-08 at 18 33 59 We could standardize the mobile navbar with the other templates by making it higher than the content on the page (i.e., adding box shadows). I think the Profile template did this well!
Screen Shot 2023-09-08 at 18 33 35 This button's border-radius doesn't seem consistent with the others used throughout the page! I guess we could also give a bit more breathing room to the container that's holding it.
Screen Shot 2023-09-08 at 18 33 05 I think we should make sure there's horizontal alignment here of the top-positioned elements. Maybe standardize the padding/sizes throughout?

Copy link
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

👍

@zanivan zanivan merged commit 3c29f0a into mui:master Sep 12, 2023
5 checks passed
@zanivan zanivan added this to the Joy UI: Stable release milestone Sep 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design: joy This is about Joy Design, please involve a visual or UX designer in the process docs Improvements or additions to the documentation package: joy-ui Specific to @mui/joy
Projects
Status: Recently completed
Development

Successfully merging this pull request may close these issues.

5 participants