[SignInPage] Allow slotProps to override all labels#4418
[SignInPage] Allow slotProps to override all labels#4418bharatkashyap merged 15 commits intomui:masterfrom
Conversation
mnajdova
left a comment
mnajdova
left a comment
There was a problem hiding this comment.
What was the reason for not using the label prop on the TextField and rendering instead a different label component?
Also, when spreading the slotProps, make sure that the complex prop are being merged, like slotProps, className, style prop etc.
Agree, no reason to be doing it this way.
Got it, I've updated to use a common function that can take care of merging props for all of the |
| htmlInput: { | ||
| sx: { | ||
| paddingTop: theme.spacing(1), | ||
| paddingBottom: theme.spacing(1), | ||
| }, | ||
| ...baseProps.slotProps?.htmlInput, | ||
| }, | ||
| inputLabel: { | ||
| sx: { | ||
| lineHeight: theme.typography.pxToRem(12), | ||
| fontSize: theme.typography.pxToRem(14), | ||
| }, | ||
| ...baseProps.slotProps?.inputLabel, | ||
| }, |
There was a problem hiding this comment.
| htmlInput: { | |
| sx: { | |
| paddingTop: theme.spacing(1), | |
| paddingBottom: theme.spacing(1), | |
| }, | |
| ...baseProps.slotProps?.htmlInput, | |
| }, | |
| inputLabel: { | |
| sx: { | |
| lineHeight: theme.typography.pxToRem(12), | |
| fontSize: theme.typography.pxToRem(14), | |
| }, | |
| ...baseProps.slotProps?.inputLabel, | |
| }, | |
| htmlInput: { | |
| ...baseProps.slotProps?.htmlInput, | |
| sx: [ | |
| { | |
| paddingTop: theme.spacing(1), | |
| paddingBottom: theme.spacing(1), | |
| }, | |
| ...(Array.isArray(baseProps.slotProps?.htmlInput?.sx) ? baseProps.slotProps?.htmlInput?.sx : [baseProps.slotProps?.htmlInput?.sx | |
| }, | |
| inputLabel: { | |
| ...baseProps.slotProps?.inputLabel, | |
| sx: [ | |
| { | |
| lineHeight: theme.typography.pxToRem(12), | |
| fontSize: theme.typography.pxToRem(14), | |
| }, | |
| ...(Array.isArray(baseProps.slotProps?.inputLabel?.sx) ? baseProps.slotProps?.inputLabel?.sx : [baseProps.slotProps?.inputLabel?.sx | |
| ], | |
| }, | |
| }, |
You can extract the sx prop into variables so we don't repeat the expressions.
mnajdova
left a comment
mnajdova
left a comment
There was a problem hiding this comment.
Left one last comment, the rest looks much better 👌
| @@ -40,6 +39,29 @@ import FusionAuthIcon from './icons/FusionAuth'; | |||
| import { BrandingContext, RouterContext } from '../shared/context'; | |||
| import { DocsContext } from '../internal/context'; | |||
There was a problem hiding this comment.
This looks like a red flag (making the logic depend on something related to the docs), unless I am misunderstanding the usage.
There was a problem hiding this comment.
This was done to help render demos smoothly in the docs. It's used in two places, mostly to do with UI:
- Not setting
autoFocustrue if the component is being used in the docs - Setting the loading state of the Credentials form to
falseeven if the login is unsuccessful (also the case in the docs)
Agreed that it's not the cleanest way - I can make a note to remove these and tackle that in one of the upcoming releases 👍🏻
2 participants
Uh oh!
There was an error while loading. Please reload this page.