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

[Input][joy] Simplify focus with :focus-within and add examples #37385

Merged
merged 13 commits into from
Jun 8, 2023

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented May 24, 2023

closes #37247

image

Docs


@siriwatknp siriwatknp added the package: joy-ui Specific to @mui/joy label May 24, 2023
@mui-bot
Copy link

mui-bot commented May 24, 2023

Netlify deploy preview

Bundle size report

Details of bundle changes

Generated by 🚫 dangerJS against f4694cb

@siriwatknp siriwatknp added component: text field This is the name of the generic UI component, not the React module! component: TextareaAutosize The React component. labels May 24, 2023
@siriwatknp siriwatknp requested a review from mnajdova May 24, 2023 04:55
@siriwatknp siriwatknp added the docs Improvements or additions to the documentation label May 24, 2023
@oliviertassinari
Copy link
Member

oliviertassinari commented May 24, 2023

This looks simpler 👍


I have noticed one bug related to the <input> not taking the whole space:
Screenshot 2023-05-25 at 01 41 05

when clicking on the input but not over the <input> 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.

[`&.${textareaClasses.focused}`]: {
'&:before': {
boxShadow: `inset 0 0 0 var(--Textarea-focusedThickness) var(--Textarea-focusedHighlight)`,
},
'&:focus-within::before': {
Copy link
Member

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 😅

Copy link
Member Author

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

Comment on lines -159 to -162
'&:-webkit-autofill': {
WebkitBackgroundClip: 'text', // remove autofill background
WebkitTextFillColor: 'currentColor',
},
Copy link
Member Author

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',
Copy link
Member Author

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',
Copy link
Member Author

Choose a reason for hiding this comment

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

This fixes the regression

@siriwatknp
Copy link
Member Author

@mnajdova A kindly reminder, need your review here.

@siriwatknp
Copy link
Member Author

@sai6855 @ZeeshanTamboli if you could help review this PR, that'd be great (would like to have this fix in the next release).

docs/data/joy/components/input/input.md Outdated Show resolved Hide resolved
docs/data/joy/components/input/input.md Outdated Show resolved Hide resolved
docs/data/joy/components/input/input.md Show resolved Hide resolved
docs/data/joy/components/input/FloatingLabelInput.tsx Outdated Show resolved Hide resolved
docs/data/joy/components/input/FloatingLabelInput.tsx Outdated Show resolved Hide resolved
docs/data/joy/components/textarea/textarea.md Outdated Show resolved Hide resolved
docs/data/joy/components/textarea/textarea.md Show resolved Hide resolved
@@ -212,6 +213,7 @@ const AutocompletePopupIndicator = styled(StyledIconButton as unknown as 'button
slot: 'PopupIndicator',
overridesResolver: (props, styles) => styles.popupIndicator,
})<{ ownerState: OwnerState }>(({ ownerState }) => ({
alignSelf: 'center',
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a 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>
Copy link
Member

@mnajdova mnajdova left a 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

@siriwatknp siriwatknp merged commit a33eb7c into mui:master Jun 8, 2023
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: text field This is the name of the generic UI component, not the React module! component: TextareaAutosize The React component. 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.

[Input][Joy] Regression with input backgrounds
5 participants