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.

Sign up for GitHub

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

Jump to bottom

[pickers] Create new component PickersSectionList #11352

Merged
merged 14 commits into from
Dec 15, 2023
Merged

[pickers] Create new component PickersSectionList #11352

merged 14 commits into from
Dec 15, 2023

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented Dec 8, 2023 •
edited

The idea is that useField expects PickersSectionList to be used but it does not care about PickersTextField / PickersInput.
So the Joy implementation (or unstyled one) can have whatever structure they want, as long as they use PickersSectionList.

Right now useField imports classes from PickersInput which is link to Material Design.

@flaviendelangle flaviendelangle self-assigned this Dec 8, 2023
@mui-bot
Copy link

mui-bot commented Dec 8, 2023 •
edited

Deploy preview: https://deploy-preview-11352--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against f48c9a8

flaviendelangle
flaviendelangle commented Dec 8, 2023
View reviewed changes
packages/x-date-pickers/src/internals/components/PickersTextField/PickersInput.tsx
outline: 'none',
flexGrow: 1,
...(theme.direction === 'rtl' && { textAlign: 'right /*! @noflip */' as any }),
Copy link
Member Author

Choose a reason for hiding this comment

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

Small fix for RTL

packages/x-date-pickers/src/internals/components/PickersTextField/PickersInput.tsx Outdated
@@ -70,11 +72,13 @@ const PickersInputSectionsContainer = styled('div', {
slot: 'SectionsContainer',
overridesResolver: (props, styles) => styles.sectionsContainer,
})<{ ownerState: OwnerStateType }>(({ theme, ownerState }) => ({
direction: 'ltr /*! @noflip */' as any,
Copy link
Member Author

Choose a reason for hiding this comment

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

Small fix for RTL
I try to do the value flip in JS and render always in LTR

packages/x-date-pickers/src/internals/components/PickersTextField/PickersInput.tsx
@@ -46,6 +46,8 @@ const PickersInputRoot = styled(Box, {
borderWidth: 2,
},
[`&.${pickersInputClasses.disabled}`]: {
pointerEvents: 'none',
Copy link
Member Author

Choose a reason for hiding this comment

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

We had hover effect when disabled

Copy link
Member

Choose a reason for hiding this comment

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

Do we by any chance also need appropriate adjustments to the onChange handler to not work when the field is disabled or readonly, but user manually removes the class from DOM?

Copy link
Member Author

Choose a reason for hiding this comment

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

In the onInput handler (equivalent of onChange for contentEditable elements), we have conditions for readOnly, I can add some for disabled to be more robust.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense. 👍

packages/x-date-pickers/src/PickersSectionList/pickersSectionListClasses.ts Outdated

export const pickersSectionListClasses = generateUtilityClasses<PickersSectionListClassKey>(
'MuiPickersSectionList',
['sectionContent'],
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'm adding this class because the field should never rely on PickersInput since people can pass custom ones.
I'll probably add a root one

flaviendelangle
flaviendelangle commented Dec 8, 2023
View reviewed changes
packages/x-date-pickers/src/internals/components/PickersTextField/PickersTextField.tsx
@@ -135,7 +135,7 @@ export const PickersTextField = React.forwardRef(function PickersTextField(
fullWidth={fullWidth}
inputProps={inputProps}
inputRef={inputRef}
sectionsContainerRef={sectionsContainerRef}
sectionListRef={sectionListRef}
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 renamed this prop to match the component name

@flaviendelangle flaviendelangle marked this pull request as ready for review December 8, 2023 15:00
@flaviendelangle flaviendelangle added the component: pickers This is the name of the generic UI component, not the React module! label Dec 8, 2023
@flaviendelangle flaviendelangle changed the title [pickers] Create new component PickersSectionsList [pickers] Create new component PickersSectionList Dec 8, 2023
noraleonte
noraleonte approved these changes Dec 11, 2023
View reviewed changes
Copy link
Contributor

@noraleonte noraleonte left a comment

Choose a reason for hiding this comment

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

Looks good! 🎉

Very small nitpick, what do you think about calling this component PickersSectionsList and keep the plural form?

@flaviendelangle
Copy link
Member Author

I'd be curious to have @LukasTy opinion on the naming.
For me, it's a list of thing, not a list of things, so "section list" instead of "sections list".
But I'm not a native speaker

LukasTy
LukasTy approved these changes Dec 12, 2023
View reviewed changes
Copy link
Member

@LukasTy LukasTy left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

I'd be curious to have LukasTy opinion on the naming.
For me, it's a list of thing, not a list of things, so "section list" instead of "sections list".
But I'm not a native speaker

I'm also not a native speaker, but am a bit torn on this one. 🙈
When in doubt, I like to check the number of google search results to help guide my decision. 🤷
Screenshot 2023-12-12 at 14 24 47
Screenshot 2023-12-12 at 14 24 54

In this case, "Section List" clearly takes the lead.
However, my gut also leans towards PickersSectionsLIst for this particular component, but I think that both forms are correct in this case and I do not urge changing it. 👍

packages/x-date-pickers/src/PickersSectionList/PickersSectionList.tsx Outdated Show resolved Hide resolved
packages/x-date-pickers/src/internals/components/PickersTextField/PickersInput.tsx
@@ -46,6 +46,8 @@ const PickersInputRoot = styled(Box, {
borderWidth: 2,
},
[`&.${pickersInputClasses.disabled}`]: {
pointerEvents: 'none',
Copy link
Member

Choose a reason for hiding this comment

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

Do we by any chance also need appropriate adjustments to the onChange handler to not work when the field is disabled or readonly, but user manually removes the class from DOM?

@samuelsycamore
Copy link
Member

My thoughts on the naming: I often see strung-together plurals like PickersSectionsList in the MUI codebase, and it's a pretty awkward structure by English standards. I would argue that something like this should be called PickerSectionList, where all of the words are singular. I can't cite any specific grammar rules I'm aware of that say why it should be this way, but it's what my gut says as a native speaker. I see that the Date and Time Picker naming conventions default to Pickers, and it'd probably be too much of a hassle to try to change all of those instances now (but if it were up to me, the naming would be PickerInput, PickerTextField, etc.). So for the sake of predictability, I'd opt for PickersSectionList here.

@flaviendelangle flaviendelangle merged commit 908585b into mui:next Dec 15, 2023
15 checks passed
@flaviendelangle flaviendelangle deleted the pickers-sections-list branch December 15, 2023 07:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LukasTy LukasTy LukasTy approved these changes

@noraleonte noraleonte noraleonte approved these changes

Assignees

@flaviendelangle flaviendelangle

Labels
component: pickers This is the name of the generic UI component, not the React module!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@flaviendelangle @mui-bot @samuelsycamore @LukasTy @noraleonte