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

[pickers] Use the new ownerState object in all the field components #15510

Merged
merged 6 commits into from
Nov 28, 2024

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented Nov 20, 2024

Part of #14475

  • Create a new useFieldOwnerState hook to create a FieldOwnerState object that extends PickerOwnerState
  • Use FieldOwnerState to resolve slotProps.textField, in all the field components
  • Use FieldOwnerState to resolve slotProps.root and slotProps.separator in the multi input range field components
  • Use SlotComponentPropsFromProps instead of SlotComponentProps for any slot handled by the field components
  • Correctly type all the slots of the field components, even the one exposed by the picker components

@mui-bot
Copy link

mui-bot commented Nov 20, 2024

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

Generated by 🚫 dangerJS against 262ff3a

@@ -117,6 +118,17 @@ export interface FieldRef<TSection extends FieldSection> {

export type FieldSelectedSections = number | FieldSectionType | null | 'all';

export interface FieldOwnerState extends PickerOwnerState {
Copy link
Member Author

@flaviendelangle flaviendelangle Nov 20, 2024

Choose a reason for hiding this comment

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

In v9, we should be able to access a lot more info if needed, but for now the structure of the component makes it very hard to put anything useful here.

I added the disabled and readOnly props because when used as standalone then are needed since isPickerDisabled and isPickerReadOnly are not reliable.

I think that's a good start that we can enrich in the future 👌
This object will also be used for the inputAdornment, openPickerButton and openPickerIcon slots once those are handled by the field component.

export interface TimeFieldSlots extends UseClearableFieldSlots {
/**
* Form control with an input to render the value.
* @default TextField from '@mui/material' or PickersTextField if `enableAccessibleFieldDOMStructure` is `true`.
* @default <PickersTextField />, or <TextField /> from '@mui/material' if `enableAccessibleFieldDOMStructure` is `false`.
Copy link
Member Author

Choose a reason for hiding this comment

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

Not related to this PR but I noticed this inconsistent JSDoc while doing the changes

@@ -79,9 +80,13 @@ export interface ExportedUseDesktopPickerSlotProps<
{},
PickerOwnerState
>;
textField?: SlotComponentProps<typeof TextField, {}, Record<string, any>>;
textField?: SlotComponentPropsFromProps<
Copy link
Member Author

Choose a reason for hiding this comment

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

Always great to have this kind of cleaning 👌
Now the slots exposed at the picker level will always have the right ownerState and the right props.

@flaviendelangle flaviendelangle marked this pull request as ready for review November 22, 2024 08:21
@flaviendelangle flaviendelangle added the component: pickers This is the name of the generic UI component, not the React module! label Nov 22, 2024
Copy link
Member

@michelengelen michelengelen left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -65,7 +67,7 @@ export interface MultiInputDateRangeFieldSlots {
/**
* Form control with an input to render a date.
* It is rendered twice: once for the start date and once for the end date.
* @default TextField from '@mui/material' or PickersTextField if `enableAccessibleFieldDOMStructure` is `true`.
* @default <PickersTextField />, or <TextField /> from '@mui/material' if `enableAccessibleFieldDOMStructure` is `false`.
Copy link
Member

Choose a reason for hiding this comment

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

should we wrap these in backticks? Not sure if we do this and if we are consistent with that.

Suggested change
* @default <PickersTextField />, or <TextField /> from '@mui/material' if `enableAccessibleFieldDOMStructure` is `false`.
* @default `<PickersTextField />`, or `<TextField />` from '@mui/material' if `enableAccessibleFieldDOMStructure` is `false`.

Copy link
Member Author

Choose a reason for hiding this comment

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

The @default is already inside a code block in the doc so adding backticks won't help 👍

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 28, 2024
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 28, 2024
@flaviendelangle flaviendelangle merged commit 511ad9f into mui:master Nov 28, 2024
18 checks passed
@flaviendelangle flaviendelangle deleted the field-ownerState branch November 28, 2024 14:41
LukasTy pushed a commit to LukasTy/mui-x that referenced this pull request Dec 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change component: pickers This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants