Skip to content

Conversation

bharatkashyap
Copy link
Member

@bharatkashyap bharatkashyap commented Nov 5, 2024

Default Themed

@bharatkashyap bharatkashyap added design: ui Visual design. type: enhancement It’s an improvement, but we can’t make up our mind whether it's a bug fix or a new feature. labels Nov 5, 2024
Copy link
Collaborator

@apedroferreira apedroferreira left a comment

Choose a reason for hiding this comment

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

Both versions look really cool from the images!
In the alert in the themed examples, you could highlight the test email and password somehow (bold, italic, a color, whichever looks better, probably bold).
Are you still going to add a playground? Not sure if I should be reviewing yet.

Just for headerInfo I think we can come up with a better name, other than that all looks good.

borderRadius: 1,
p: 4,
border: '1px solid',
borderColor: (theme) => alpha(theme.palette.grey[400], 0.4),
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you're using functions in properties more than once I guess the whole sx can be a function as the first argument should be the theme?

* Custom information added to the top header beneath the title
* @default Typography
*/
headerInfo?: React.ElementType;
Copy link
Collaborator

Choose a reason for hiding this comment

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

By header you mean the title, right? But we've been calling "header" to something more like the AppBar on top of pages. So maybe instead of "header" it could be "heading" or "title"?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's the section that could contain a sub-heading, given the heading of the Sign In Page is "Sign In", but I get your concern about the conflict with the "header" being the app bar. We could call it secondaryTitle?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That sounds ok, on second thought maybe "headerInfo" isn't bad either but it could get confusing if we end up adding a header with the theme switcher I guess...
Or even something like "subHeading" could work probably. Feel free to go with what you think is best in the end, just thought I'd mention this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, on second thought I could probably add two slots called title and subtitle to be able to override that entire section. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

title and subtitle sound super clear to me.

required
slotProps={{
htmlInput: {
sx: (theme) => ({
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also if we're using the theme so much, we could just call it from the useTheme hook?

@bharatkashyap
Copy link
Member Author

bharatkashyap commented Nov 6, 2024

Are you still going to add a playground? Not sure if I should be reviewing yet.

Added a playground for you to test. Run pnpm --filter playground-nextjs-themed dev. I can move this into an example once we merge this PR, what do you think?

@apedroferreira
Copy link
Collaborator

Are you still going to add a playground? Not sure if I should be reviewing yet.

Added a playground for you to test. Run pnpm --filter playground-nextjs-themed dev. I can move this into an example once we merge this PR, what do you think?

Wow it looks really good!
Yeah, i guess this makes more sense as an example than a playground so we can just move it afterwards.

Some feedback from what I tested:

  • Weird spacing above "main items" heading (too small):
Screenshot 2024-11-07 at 17 57 23
  • I forgot to set the auth secret env variable at first and it just took me to this page:
Screenshot 2024-11-07 at 17 59 38

Not sure if it should show the error in the app itself instead?

  • A border probably doesn't need to be there in the "mini" version of the sidebar account button:
Screenshot 2024-11-07 at 18 02 08

These don't need to be blocking as they're probably minor issues.
Also I'm noticing the theme can flash from light to dark when the page loads, I'll look into it as it shouldn't be happening.

@bharatkashyap
Copy link
Member Author

  • Weird spacing above "main items" heading (too small):
  • A border probably doesn't need to be there in the "mini" version of the sidebar account button:

Good catch. Fixing these!

Not sure if it should show the error in the app itself instead?

It's planned to add an AuthErrorPage component that can be used to handle these errors (potentially after the SignUpPage) for now this is intentional to allow users to handle these errors with their own UI.

@bharatkashyap bharatkashyap merged commit c8ac7fa into mui:master Nov 8, 2024
14 checks passed
@@ -15,7 +15,7 @@
"markdownlint": "markdownlint-cli2 \"**/*.md\"",
"prettier": "pretty-quick --ignore-path .eslintignore",
"prettier:all": "prettier --write . --ignore-path .eslintignore",
"dev": "dotenv cross-env FORCE_COLOR=1 lerna -- run dev --stream --parallel --ignore docs --ignore playground-nextjs --ignore playground-nextjs-pages --ignore playground-vite --ignore playground-nextjs-themed",
"dev": "dotenv cross-env FORCE_COLOR=1 lerna -- run dev --stream --parallel --ignore docs --ignore playground-*",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Cool, didn't know you could do this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design: ui Visual design. type: enhancement It’s an improvement, but we can’t make up our mind whether it's a bug fix or a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose sx for SignInPage/SignUpPage [template] [ux] SignInPage improvements [core] DashboardLayout theme overrides issues
3 participants