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

[joy-ui][docs] Remove wrong CSS prop from the Sign-in-side template #41383

Merged
merged 8 commits into from
Mar 8, 2024

Conversation

cipherlogs
Copy link
Contributor

@cipherlogs cipherlogs commented Mar 6, 2024

The term align-items: left doesn’t exist and there’s no need to worry about cross axis alignment because there’s no extra height. So, you can remove it from the code without any issues.

👉 https://deploy-preview-41383--material-ui.netlify.app/joy-ui/getting-started/templates/sign-in-side/

The term align-items: left doesn’t exist and also There’s no need to worry about cross axis alignment because there’s no extra height. So, you can remove it from the code without any issues.

Signed-off-by: Nedal <[email protected]>
@mui-bot
Copy link

mui-bot commented Mar 6, 2024

Netlify deploy preview

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

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 7f3bd71

@zannager zannager added docs Improvements or additions to the documentation package: joy-ui Specific to @mui/joy labels Mar 6, 2024
@zannager zannager requested a review from zanivan March 6, 2024 12:02
Copy link
Contributor Author

@cipherlogs cipherlogs left a comment

Choose a reason for hiding this comment

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

The current code uses clamp to switch between two values, which seems complicated for no reason. Since we already have access to the theme object, we could simplify this to make the code easier to read and reason about.

When using light mode, there’s an issue between the `769px` and `900px` width range. During this range, the divider is not visible. I recommend removing the collapsing breakpoint altogether and relying solely on the defaul `md` value
breakpoint.
@cipherlogs cipherlogs marked this pull request as draft March 6, 2024 13:12
There's no need for an extra import, we can use the class name directly to target all asterisk slots
withing the form container.
Better provide a meaningful feedback to users and screen readers, regardless of whether the component is mounted or not. Here are the changes made:

1. When component isn't mounted
  + an aria-label and a default icon will be provided for better context.
  + the button will be disabled
  + `onClick` will be attached, but it won't be invoked because the button is disabled

2. When component is mounted
  + The user will be able to toggle the color scheme as expected
@cipherlogs cipherlogs marked this pull request as ready for review March 6, 2024 16:24
Copy link
Contributor

@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.

Good catch! Thanks for tackling this! 🚀

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.

👍 Great first PR!

@siriwatknp siriwatknp enabled auto-merge (squash) March 8, 2024 03:43
auto-merge was automatically disabled March 8, 2024 08:54

Head branch was pushed to by a user without write access

@danilo-leal danilo-leal changed the title [joy-ui][docs] Remove wrong css prop [joy-ui][docs] Remove wrong CSS prop from the Sign-in-side template Mar 8, 2024
@zanivan zanivan merged commit a774510 into mui:master Mar 8, 2024
19 checks passed
mnajdova pushed a commit to mnajdova/material-ui that referenced this pull request Mar 8, 2024
@cipherlogs cipherlogs deleted the patch-1 branch March 8, 2024 15:15
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 package: joy-ui Specific to @mui/joy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants