-
-
Notifications
You must be signed in to change notification settings - Fork 32.5k
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
[Input][joy] Simplify focus with :focus-within
and add examples
#37385
Conversation
Netlify deploy previewBundle size report |
This looks simpler 👍 I have noticed one bug related to the when clicking on the input but not over the |
[`&.${textareaClasses.focused}`]: { | ||
'&:before': { | ||
boxShadow: `inset 0 0 0 var(--Textarea-focusedThickness) var(--Textarea-focusedHighlight)`, | ||
}, | ||
'&:focus-within::before': { |
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.
One point of complication, on https://mui.com/x/react-date-pickers/date-range-picker/#basic-usage, we found a use case to set the focus style without having the input actually DOM focused 😅
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.
I have noticed one bug related to the
<input>
not taking the whole space:
pushed a fix!
when clicking on the input but not over the the focus is only set on mouse up, I think that it should be on mouse down. I did it wrong on the Autocomplete too, e.g. https://mui.com/material-ui/react-autocomplete/#combo-box, not a great UX.
This should be fixed from Base UI's useInput
hook
One point of complication, on https://mui.com/x/react-date-pickers/date-range-picker/#basic-usage, we found a use case to set the focus style without having the input actually DOM focused 😅
I oppose the use of a prop for this case and I think a CSS variable sounds better. Here is how you toggle the focus ring in Joy UI:
<Input sx={{ '--Input-focused': 1 }} />
added to the docs: https://deploy-preview-37385--material-ui.netlify.app/joy-ui/react-input/#triggering-the-focus-ring
'&:-webkit-autofill': { | ||
WebkitBackgroundClip: 'text', // remove autofill background | ||
WebkitTextFillColor: 'currentColor', | ||
}, |
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.
textarea does not have autofill
@@ -200,6 +200,7 @@ const AutocompleteClearIndicator = styled(StyledIconButton as unknown as 'button | |||
slot: 'ClearIndicator', | |||
overridesResolver: (props, styles) => styles.clearIndicator, | |||
})<{ ownerState: OwnerState }>(({ ownerState }) => ({ | |||
alignSelf: 'center', |
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.
This is required because the Input
's root no longer have align-items: center
.
@@ -89,7 +90,6 @@ export const StyledInputRoot = styled('div')<{ ownerState: InputOwnerState }>( | |||
cursor: 'text', | |||
position: 'relative', | |||
display: 'flex', | |||
alignItems: 'center', |
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.
This fixes the regression
@mnajdova A kindly reminder, need your review here. |
@sai6855 @ZeeshanTamboli if you could help review this PR, that'd be great (would like to have this fix in the next release). |
@@ -212,6 +213,7 @@ const AutocompletePopupIndicator = styled(StyledIconButton as unknown as 'button | |||
slot: 'PopupIndicator', | |||
overridesResolver: (props, styles) => styles.popupIndicator, | |||
})<{ ownerState: OwnerState }>(({ ownerState }) => ({ | |||
alignSelf: 'center', |
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.
StyledIconButton already has alignItems: center
.
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.
alignItems: center
is for flexbox container but alignSelf: center
is for flexbox item.
Added alignSelf: 'center'
here to make the popup indicator stay in the middle of the input.
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.
Yes I know that. But I thought there will always be one item inside the popup indicator button.
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.
Looks great!
Co-authored-by: Zeeshan Tamboli <zeeshan.tamboli@gmail.com> Signed-off-by: Siriwat K <siriwatkunaporn@gmail.com>
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.
Looks great! 👍 for the new examples
closes #37247
Docs