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 context to pass props from the picker to the field #16042

Open
wants to merge 25 commits into
base: master
Choose a base branch
from

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented Dec 31, 2024

Part of #16059

This PR only cover the props that are common to all the fields, otherwise it would be too big and hard to review.
For now the validation props and some other prop are still passed to the field using props (see "Follow up" for more details).

  • Create a new private context for the properties that are specific to our editing mechanism (let me know if you think those should also be in the public context.
  • Use disabled, value and setValue from the existing public context in the field components
  • Add fieldFormat and timezone to the existing public context and use them in the field components
  • Remove formatDensity and shouldRespectLeadingZeros from useParsedFormat, and make format optional (use the one passed by the picker by default).
  • Migrate all the doc demos to use the context instead of internal props for the migrated properties.
  • Write migration guide

Follow up

@flaviendelangle flaviendelangle added breaking change component: pickers This is the name of the generic UI component, not the React module! labels Dec 31, 2024
@flaviendelangle flaviendelangle self-assigned this Dec 31, 2024
@flaviendelangle flaviendelangle marked this pull request as draft December 31, 2024 08:34
@mui-bot
Copy link

mui-bot commented Dec 31, 2024

@flaviendelangle flaviendelangle changed the title [pickers] Use context to pass props specific to our field editing behavior from the picker [pickers] Use context to pass props from the picker to the field Dec 31, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 31, 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 Dec 31, 2024
* @param {string} params.format Format of the date to use.
* @param {'dense' | 'spacious'} params.formatDensity Density of the format (setting `formatDensity` to `"spacious"` will add a space before and after each `/`, `-` and `.` character).
* @param {boolean} params.shouldRespectLeadingZeros If `true`, the format will respect the leading zeroes, if `false`, the format will always add leading zeroes.
* @param {string} params.format Format to parse.
Copy link
Member Author

Choose a reason for hiding this comment

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

  • shouldRespectLeadingZeros: I removed it because it was doing nothing since it's a placeholder and a placeholder never has leading zeros 🤦 .

  • formatDensity: I removed it, but for this one it's more subjective and I would like your opinion. I can take it from the context and allow an override like for the format. My reasoning behind the removal is that the density is a concept internal to the our field. If someone uses it in a custom field (let say a masked field), then he/she will have an inconsistency between the placeholder (which will have the spaces) and the value (which wont). So I think it's better to just let the user use a good old format.replace(/\//g, ' / ').

  • format: I made it optional, by default it's using the one of the picker (or a default date format when using a standalone field but it's probably not a realistic use case)

Copy link
Member

Choose a reason for hiding this comment

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

On the formatDensity I do agree ... its probably best to give the user the freedom to use whatever density he prefers and apply it to both. 👍🏼

@@ -41,16 +40,18 @@ function ButtonDateTimeField(props: DateTimePickerFieldProps) {
} = forwardedProps;

const pickerContext = usePickerContext();

const parsedFormat = useParsedFormat(internalProps);
const parsedFormat = useParsedFormat();
const { hasValidationError } = useValidation({
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 would now be able to make value and timezone optional and read it from the context by default. Same with the validation props in the future.

But I would like to take the time to think this API correctly before doing anything.

@flaviendelangle flaviendelangle marked this pull request as ready for review January 1, 2025 13:18
* @param {string} params.format Format of the date to use.
* @param {'dense' | 'spacious'} params.formatDensity Density of the format (setting `formatDensity` to `"spacious"` will add a space before and after each `/`, `-` and `.` character).
* @param {boolean} params.shouldRespectLeadingZeros If `true`, the format will respect the leading zeroes, if `false`, the format will always add leading zeroes.
* @param {string} params.format Format to parse.
Copy link
Member

Choose a reason for hiding this comment

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

On the formatDensity I do agree ... its probably best to give the user the freedom to use whatever density he prefers and apply it to both. 👍🏼

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

github-actions bot commented Jan 7, 2025

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 Jan 7, 2025
- `onSelectedSectionsChange`: `undefined`

:::success
If you are using a hook like `useDateField`, you don't have to do anything, the value from the context are automatically applied.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
If you are using a hook like `useDateField`, you don't have to do anything, the value from the context are automatically applied.
If you are using a hook, like `useDateField`, you don't have to do anything, the values from the context are automatically applied.

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 might have applied the comment before too fast.
It's not all hooks, only some of them.

For me If your are using a hook, like useDateField, you ... implies that the condition if "If you are using a hook", which is not accurate here.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, sorry for misleading you. 🙈

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.

4 participants