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

[website] Fix a few regression #38050

Merged
merged 18 commits into from
Jul 21, 2023
Merged

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Jul 19, 2023

@oliviertassinari oliviertassinari added the website Pages that are not documentation-related, marketing-focused. label Jul 19, 2023
@oliviertassinari oliviertassinari force-pushed the batch-fixes branch 2 times, most recently from 3649651 to 2f5d308 Compare July 19, 2023 15:44
@mui-bot
Copy link

mui-bot commented Jul 19, 2023

@@ -228,7 +246,7 @@ export default function MarkdownDocsV2(props) {
>
{disableAd ? null : (
<BrandingProvider>
<AdGuest classSelector=".component-tabs">
<AdGuest classSelector={hasTabs ? '.component-tabs' : undefined}>
Copy link
Member Author

Choose a reason for hiding this comment

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

@@ -86,7 +86,7 @@ export default function DiamondSponsors() {
(theme) => ({
'& a': {
width: '100%',
height: { xs: 40, sm: 50 },
height: { xs: 40, sm: 45 },
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm struggling with the diamond sponsors at the bottom, the UX issue for me is when I expand a side nav I don't have enough vertical height to browse the list comfortably (desktop)

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems aligned with what a community member is feeling too (#38033)... I'm considering changing it.

@@ -47,7 +47,7 @@ export default function HeroContainer({

const renderRightWrapper = (sx?: BoxProps['sx']) => (
<Box
id="hero-container-right-area"
ref={frame}
Copy link
Member Author

Choose a reason for hiding this comment

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

Open https://mui.com/, you can Tab between the different elements in the hero.

Comment on lines 38 to 40
<Typography variant="h2" sx={{ textAlign: 'center' }} gutterBottom>
Ready to use components,
<br />
Copy link
Member Author

Choose a reason for hiding this comment

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

There is a missing space in https://mui.com/core/, but new line feels even better.

Copy link
Contributor

Choose a reason for hiding this comment

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

Much better, good catch!

@@ -76,7 +76,7 @@ function ProductItem({
event.stopPropagation();
}}
>
<span>Learn more</span>{' '}
<span>Learn more</span> <Box sx={visuallyHidden}>{label}</Box>
Copy link
Member Author

Choose a reason for hiding this comment

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

Comment on lines 1 to +12
import * as React from 'react';
import MarkdownDocs from 'docs/src/modules/components/MarkdownDocs';
import MarkdownDocs from 'docs/src/modules/components/MarkdownDocsV2';
import AppFrame from 'docs/src/modules/components/AppFrame';
import * as pageProps from 'docs/data/base/getting-started/customization/customization.md?@mui/markdown';

export default function Page() {
return <MarkdownDocs {...pageProps} />;
}

Page.getLayout = (page) => {
return <AppFrame>{page}</AppFrame>;
};
Copy link
Member Author

Choose a reason for hiding this comment

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

What I believe we should do on ALL the pages of the docs. It's a continuation from #35938 (comment).

Copy link
Member Author

Choose a reason for hiding this comment

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

@alexfauquette I have tested https://www.notion.so/mui-org/Migrate-all-pages-to-Page-getLayout-212d920a9ab245f08ac3a2aea874f1bc with a few more pages. I had to fix 2 bugs in MarkdownDocsV2 (ad & codeblocks) but so far so good. The improvement is quite noticeable (before -> after):

Screenshot 2023-07-19 at 22 56 04 Screenshot 2023-07-19 at 22 54 54

I also love how the layout is stable (no more search flash, no more badge notification flash, etc.)

It will be a great step for https://www.notion.so/mui-org/Docs-page-transition-performance-regression-b9e887541f5e4380b991ba173f4c0f6e but I doubt it will be enough. Maybe Romain would enjoy this challenge, he has proven on the data grid that he's more than capable 😁.

@oliviertassinari oliviertassinari force-pushed the batch-fixes branch 3 times, most recently from 46b004d to 135d38d Compare July 19, 2023 16:29
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.

Some tiny considerations!

docs/src/components/home/ProductsSwitcher.tsx Outdated Show resolved Hide resolved
@@ -86,7 +86,7 @@ export default function DiamondSponsors() {
(theme) => ({
'& a': {
width: '100%',
height: { xs: 40, sm: 50 },
height: { xs: 40, sm: 45 },
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems aligned with what a community member is feeling too (#38033)... I'm considering changing it.

color: 'primary.300',
borderColor: 'primaryDark.600',
backgroundColor: alpha(theme.palette.primaryDark[700], 0.4),
backgroundColor: theme.palette.primaryDark[700],
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason to remove the background opacity?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm outdoor, I could not see the difference, but no objections to revert.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm happy to change it, just wanted to know if there was a reason!

Comment on lines -82 to -90
{rendered.map((renderedMarkdownOrDemo, index) => {
if (typeof renderedMarkdownOrDemo === 'string') {
return (
<Wrapper key={index} {...(isJoy && { mode: theme.palette.mode })}>
<MarkdownElement renderedMarkdown={renderedMarkdownOrDemo} />
</Wrapper>
);
}

Copy link
Member Author

Choose a reason for hiding this comment

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

This duplication was not great, we introduced the support for "codeblock" here and forgot about the V2 🙈

Comment on lines -4 to -11

export default function DemosDocs(props) {
const { WrapperComponent: Wrapper, wrapperProps, rendered = [], ...rest } = props;
return (
<React.Fragment key="demos-docs">
{rendered.map((renderedMarkdownOrDemo, index) => {
return (
<MarkdownElement
Copy link
Member Author

Choose a reason for hiding this comment

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

dead code

activeTab={activeTab}
setActiveTab={setActiveTab}
Copy link
Member Author

Choose a reason for hiding this comment

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

Dead code

Comment on lines -40 to +41
mt: 1,
pt: 0.5,
pb: 0.5,
Copy link
Member Author

Choose a reason for hiding this comment

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

Increase the link surface area for mobile:

Screenshot 2023-07-19 at 18 48 36

It's also more enjoyable on desktop, no click gap when you move the cursor.

We go from 21px to 29px without visual changes. I haven't tried to hit 48px.

Copy link
Contributor

Choose a reason for hiding this comment

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

Much better indeed!

Comment on lines -119 to +135
MarkdownElementV2.propTypes = {
RichMarkdownElement.propTypes = {
Copy link
Member Author

Choose a reason for hiding this comment

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

The name wa wrong, it's not the same level of abstraction.

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.

Thanks for explaining some of the changes in the comments ⎯ it's a good learning resource! 🤙

@oliviertassinari oliviertassinari merged commit 71eea59 into mui:master Jul 21, 2023
21 checks passed
@oliviertassinari oliviertassinari deleted the batch-fixes branch July 21, 2023 19:33
@oliviertassinari
Copy link
Member Author

I'm excited about these fixes, it's cleaner 😌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
website Pages that are not documentation-related, marketing-focused.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants