-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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] Set closeOnSelect
prop default value to false
on pickers containing time views
#14397
base: master
Are you sure you want to change the base?
Changes from 28 commits
1107519
887eef1
a4c7ee5
cf44c34
f77003f
8adfacd
9ad1a66
243ba59
85994db
93c9a64
71430c2
32fe2fc
5765830
b4d0dd2
89a4fea
65df2e8
3d6ab99
0b48a93
2ecd55a
24ac4f8
06b1cdd
be35b9b
34c1cce
ec4269c
0bb8cc1
9ab2c7d
8af50b8
26f9e34
37dcf54
2a2e1f9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -51,9 +51,12 @@ In all the below scenarios, the picker closes when `onClose` is called, except i | |||||
#### When the last view is completed | ||||||
|
||||||
When a selection in the last view is made, `onClose` will be called only if the `closeOnSelect` prop is equal to `true`. | ||||||
By default, it is set to `true` on desktop and `false` on mobile. | ||||||
The default value of `closeOnSelect` varies among the components: | ||||||
|
||||||
Here are a few examples: | ||||||
- `DatePicker` and `DateRangePicker`: `true` on desktop and `false` on mobile variants; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
- `TimePicker`, `DateTimePicker` and `DateTimeRangePicker`: `false` on both desktop and mobile variants. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
||||||
Here are a few examples: | ||||||
|
||||||
:::info | ||||||
The examples below are using the desktop and mobile variants of the pickers, but the behavior is exactly the same when using the responsive variant (`DatePicker`, `TimePicker`, ...) on a mobile or desktop environment. | ||||||
|
@@ -225,7 +228,10 @@ You can use the second argument passed to the `onAccept` callback to get the val | |||||
#### When the last view is completed | ||||||
|
||||||
When a selection in the last view is made, `onAccept` will be called only if the `closeOnSelect` prop is equal to `true` and the value has been modified since the last time `onAccept` was called. | ||||||
By default, `closeOnSelect`, is set to `true` on desktop and `false` on mobile. | ||||||
The default value of `closeOnSelect` varies among the components: | ||||||
arthurbalduini marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same changes as proposed above |
||||||
|
||||||
- `DatePicker` and `DateRangePicker`: `true` on desktop and `false` on mobile variants; | ||||||
- `TimePicker`, `DateTimePicker` and `DateTimeRangePicker`: `false` on both desktop and mobile variants. | ||||||
Comment on lines
+233
to
+234
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same as @flaviendelangle 's suggestion above: Date Picker, Date Range Picker, etc. |
||||||
|
||||||
Here are a few examples: | ||||||
|
||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,7 +3,7 @@ | |
"autoFocus": { "type": { "name": "bool" } }, | ||
"closeOnSelect": { | ||
"type": { "name": "bool" }, | ||
"default": "`true` for desktop, `false` for mobile (based on the chosen wrapper and `desktopModeMediaQuery` prop)." | ||
"default": "`true` for desktop, `false` for mobile variant (based on the chosen wrapper and `desktopModeMediaQuery` prop)." | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this supposed to be singular here but plural in the next one? |
||
}, | ||
"dayOfWeekFormatter": { | ||
"type": { "name": "func" }, | ||
|
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -48,6 +48,11 @@ export interface DateRangePickerProps< | |||||||||
* @default {} | ||||||||||
*/ | ||||||||||
slotProps?: DateRangePickerSlotProps<TDate, TEnableAccessibleFieldDOMStructure>; | ||||||||||
/** | ||||||||||
* If `true`, the popover or modal will close after submitting the full date. | ||||||||||
* @default `true` for desktop, `false` for mobile variants (based on the chosen wrapper and `desktopModeMediaQuery` prop). | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we are not deciding between
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it reads more clearly without "variant" so unless it's absolutely necessary for clarity then I'd leave it out. |
||||||||||
*/ | ||||||||||
closeOnSelect?: boolean; | ||||||||||
} | ||||||||||
|
||||||||||
/** | ||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -122,6 +122,11 @@ export interface BaseDateTimeRangePickerProps<TDate extends PickerValidDate> | |||||||||||
* If `undefined`, internally defined view will be used. | ||||||||||||
*/ | ||||||||||||
viewRenderers?: Partial<DateTimeRangePickerRenderers<TDate, DateTimeRangePickerView>>; | ||||||||||||
/** | ||||||||||||
* If `true`, the popover or modal will close after submitting the full date. | ||||||||||||
* @default false | ||||||||||||
*/ | ||||||||||||
closeOnSelect?: boolean; | ||||||||||||
Comment on lines
+119
to
+123
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we could update the JSDoc of this prop on
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The less override we have the better 🙏 |
||||||||||||
} | ||||||||||||
|
||||||||||||
type UseDateTimeRangePickerDefaultizedProps< | ||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -44,4 +44,9 @@ export interface DesktopDateRangePickerProps< | |||||||||
* @default {} | ||||||||||
*/ | ||||||||||
slotProps?: DesktopDateRangePickerSlotProps<TDate, TEnableAccessibleFieldDOMStructure>; | ||||||||||
/** | ||||||||||
* If `true`, the popover or modal will close after submitting the full date. | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we have a specific definition per component, we know its default behavior.
Suggested change
Otherwise, we could go the more abstract route to avoid the need to update the JSDoc when we move range pickers away from the tooltip to dialog/popover. 🤔
Suggested change
|
||||||||||
* @default true | ||||||||||
*/ | ||||||||||
closeOnSelect?: boolean; | ||||||||||
} |
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -54,6 +54,11 @@ export interface DatePickerProps< | |||||||||
* @default 4 on desktop, 3 on mobile | ||||||||||
*/ | ||||||||||
yearsPerRow?: 3 | 4; | ||||||||||
/** | ||||||||||
* If `true`, the popover or modal will close after submitting the full date. | ||||||||||
* @default `true` for desktop, `false` for mobile variant (based on the chosen wrapper and `desktopModeMediaQuery` prop). | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not fan of just adding
Suggested change
or
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We currently call them "variants" in the doc as well. 🙈 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion updated 😆 |
||||||||||
*/ | ||||||||||
closeOnSelect?: boolean; | ||||||||||
} | ||||||||||
|
||||||||||
/** | ||||||||||
|
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.
But I'll let @samuelsycamore check the wordings 🙏
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'm happy with your version @flaviendelangle 👍