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] Refine checkout template #40967

Merged
merged 72 commits into from Mar 8, 2024

Conversation

zanivan
Copy link
Contributor

@zanivan zanivan commented Feb 6, 2024

Part of #37555 / Closes #41098

This PR is for refining the checkout template to be aligned with the new landing page template.

Current New
image image

👉 https://deploy-preview-40967--material-ui.netlify.app/material-ui/getting-started/templates/checkout/

Note

The thumbnails and template's information will be updated last

@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 Feb 6, 2024
@zanivan zanivan self-assigned this Feb 6, 2024
@mui-bot
Copy link

mui-bot commented Feb 6, 2024

Netlify deploy preview

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

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 257acbb

@zanivan zanivan changed the title [material-ui] Refining checkout template [material-ui] Refine checkout template Feb 6, 2024
@mbrookes mbrookes self-requested a review February 15, 2024 18:07
@zanivan zanivan marked this pull request as ready for review February 19, 2024 19:23
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.

We're getting there! Large review but mostly details. Also, heads-up to add theme customizations to the Alert component (that's too Material Design-y on the custom theme!)

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.

Looking great! Excited to have this one out, too — further improvements can come iteratively, in my perspective 🤙 And thanks for being so welcoming with the extensive feedback! 🙏

Copy link
Contributor Author

@zanivan zanivan left a comment

Choose a reason for hiding this comment

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

@DiegoAndai I think this is ready for the engineering review 🙌 Would you mind giving me a hand with the failing tests?

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 this file was created by docs:typescript:formatted, but I don't it's necessary. However, the tests fail when this file is not committed—what could be done then?

Copy link
Member

Choose a reason for hiding this comment

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

Weird 🤔 InfoMobile.tsx is the only file this is happening with? It looks like it's mistaking it for a docs demo. @mnajdova would you know anything about this?

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 only create preview files for the demos, not templates. I will look into updating the script. It was likely generated only for this file because there render is short (less than 15 lines of code I think was the minimum :))

Copy link
Member

Choose a reason for hiding this comment

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

Fixed in #41379. @zanivan you should remove the file after this PR is merged and it won't be generated anymore.

@zanivan zanivan requested a review from DiegoAndai March 5, 2024 13:26
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.

Looks good 🚀

Is the regression test failing consistently?

@DiegoAndai
Copy link
Member

DiegoAndai commented Mar 6, 2024

For the regression test failure, we need to exclude the getCheckoutTheme file as we did with the landing page: 11d46c7

We have to do this for all non-component files: #37557 (comment)

@zanivan zanivan merged commit fe5fb08 into mui:master Mar 8, 2024
22 checks passed
mnajdova pushed a commit to mnajdova/material-ui that referenced this pull request Mar 8, 2024
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][templates] Checkbox is barely visible when checked on the Checkout template
7 participants