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] Markdown redesign polish #27956

Merged
merged 84 commits into from
Sep 1, 2021

Conversation

mnajdova
Copy link
Member

@mnajdova mnajdova commented Aug 25, 2021

Opening early so that we can iterate with @danilo-leal

Preview link: https://deploy-preview-27956--material-ui.netlify.app/

@mui-pr-bot
Copy link

mui-pr-bot commented Aug 25, 2021

No bundle size changes (experimental)

Generated by 🚫 dangerJS against 535684f

@danilo-leal

This comment has been minimized.

@mnajdova
Copy link
Member Author

@danilo-leal sometimes some of the docs component are being rendered as part of the Demos (note that in that case they have the Material Design theme available, no the branding). This is why sometimes you were seeing errors that primaryDark does not exists. So far, I've found the issues in the MarkdownElement and replaced those usages by simply importing directly the color (darkBlue). You can see the changes in 58c1436

@siriwatknp I've also updated how the new theme is created, no need to invoke two times createTheme is enough to just deep merge the prev result with the one coming from getThemeComponents. See b2a751d

@siriwatknp

This comment has been minimized.

@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 31, 2021

I have pushed small commits trying to simplify and fix more issues, for instance, we were using a font that we don't load, and since loading extra fonts comes with a cost, I defaulted to using the existing ones. @danilo-leal I think that some of these changes could be used as a review of the changes.

The last thing I could notice is the lack of nested side nav. We need it for the date picker, and soon for scaling the content of the data grid. The regression:

Screenshot 2021-08-31 at 18 43 38

@oliviertassinari

This comment has been minimized.

@@ -81,7 +81,7 @@ const Root = styled('div')(({ theme }) => ({
'& h3': {
...theme.typography.h6,
margin: '20px 0 10px',
fontFamily: ['"PlusJakartaSans-Bold"'].join(','),
fontFamily: ['"PlusJakartaSans-ExtraBold"'].join(','),
Copy link
Contributor

Choose a reason for hiding this comment

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

@oliviertassinari Any idea on how we can use the bold weight? It hurts the heading hierarchy a bit having the h3 styling be extra-bold too.

Copy link
Member

@oliviertassinari oliviertassinari Aug 31, 2021

Choose a reason for hiding this comment

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

I think that we can ask @siriwatknp. He seems to have been iterating multiple times on the best way to load custom fonts.

For context, I noticed it loading the page in BrowserStack in a Windows 10 machine, where this font-weight is not installed by default. The header had the default user-agent font, not pretty.

Copy link
Member

Choose a reason for hiding this comment

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

will add the bold weight.

@danilo-leal
Copy link
Contributor

@oliviertassinari

Great point about the nested Date/Time. I tried to tackle it myself but couldn't pull it off. @mnajdova or @siriwatknp could definitely pull it off easily. I think maybe something like this works for the time being: having the Date / Time "parent" be in the same style as the other ones and just push its children a bit to the left. What do you think?

Screen Shot 2021-08-31 at 14 43 58

@mbrookes
Copy link
Member

How should we handle the component sub-section reorder? I can add a commit to this PR per the Figma proposal, or submit a PR to the PR for further discussion...

@siriwatknp
Copy link
Member

@mnajdova @oliviertassinari @mbrookes @danilo-leal What do you think if we merge the current progress so that it goes with this week release and then create another PR for what's left?

@mnajdova
Copy link
Member Author

mnajdova commented Sep 1, 2021

What do you think if we merge the current progress so that it goes with this week release and then create another PR for what's left?

Sounds good, I will merge this after the package rename, and then we can do the release.

@mnajdova mnajdova marked this pull request as ready for review September 1, 2021 06:47
@mnajdova
Copy link
Member Author

mnajdova commented Sep 1, 2021

@mnajdova It seems a bit off having the language menu between the search input and the other icon buttons. My train of thought would be to have all constrained on visible containers (borders) interactive options closer than farther. Do you think there's another way to fix the layout shift (bullet nº5) without changing the component order?

Screen Shot 2021-08-31 at 12 59 21

@danilo-leal maybe we can move the language at the end (after the settings button)?

Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

Demo code is readable again, thanks 👍🏻

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 1, 2021
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 1, 2021
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.

7 participants