-
-
Notifications
You must be signed in to change notification settings - Fork 315
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
[SignInPage] Rename rememberMe
slot to checkbox
#4574
base: master
Are you sure you want to change the base?
Conversation
Netlify deploy preview |
rememberMe
optional, add form
slotProprememberMe
optional
rememberMe
optionalrememberMe
slot to checkbox
rememberMe
slot to checkbox
rememberMe
slot to checkbox
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Took a look and left some comments!
Overall the idea of the "remember me" slot being more generic and optional with its own component seems good to me.
emailField: { variant: 'standard', autoFocus: false }, | ||
passwordField: { variant: 'standard' }, | ||
submitButton: { variant: 'outlined' }, | ||
rememberMe: { | ||
checkbox: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
checkbox
is more generic than rememberMe
so I think it's an improvement, but could we make it more generic even as any type of content could go in this slot, such as a small disclaimer for example, or multiple checkboxes?
Not sure if something like formFooter
would be a good name...
Anyway these are just some thoughts!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I see we have a forgotPasswordLink
slot too, so if when implementing these you saw that there's a benefit to these slots being more specific then it should be fine.
@@ -16,17 +16,19 @@ export default function SlotPropsSignIn() { | |||
`Signing in with "${provider.name}" and credentials: ${formData.get('email')}, ${formData.get('password')} and checkbox value: ${formData.get('tandc')}`, | |||
) | |||
} | |||
slots={{ checkbox: RememberCheckbox }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about RememberMeCheckbox
, would it be just a bit more descriptive?
@@ -239,7 +239,7 @@ To enable deep customization beyond what is possible with custom props, the `Sig | |||
|
|||
{{"demo": "SlotsSignIn.js", "iframe": true, "height": 540 }} | |||
|
|||
You can use the `slotProps` prop to pass props to the underlying components of each slot: | |||
You can use the `slotProps` prop to pass props to the underlying components of each slot, and also to `form` element: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use the `slotProps` prop to pass props to the underlying components of each slot, and also to `form` element: | |
You can use the `slotProps` prop to pass props to the underlying components of each slot, and also to the `form` element: |
@@ -18,10 +18,10 @@ | |||
}, | |||
"classDescriptions": {}, | |||
"slotDescriptions": { | |||
"checkbox": "A custom checkbox placed in the credentials form", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we change the name as I proposed we'd have to update the description to something more generic too.
color: 'textSecondary', | ||
fontSize: theme.typography.pxToRem(14), | ||
}, | ||
...props?.slotProps?.typography, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we just spread this inside typography
instead?
import { FormControlLabel, Checkbox } from '@mui/material'; | ||
import { useTheme } from '@mui/material/styles'; | ||
|
||
interface RememberCheckboxProps { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we just allow all slotProps
from FormControlLabel
and make sure they're passed to the component?
Breaking Change
rememberMe
tocheckbox
to make the slot more genericcheckbox
slotOther Changes
form
slotProp
to enable passing attributes to the containingform
elements, such asnoValidate: true
in case someone wants to disable the client-side form validation