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] Implement clearable field behavior #9095

Merged
merged 73 commits into from
Sep 28, 2023

Conversation

noraleonte
Copy link
Contributor

@noraleonte noraleonte commented May 23, 2023

Closes #4450
Closes #10492

Add a clearable prop to the pickers

Clearable behavior

@noraleonte
Copy link
Contributor Author

@flaviendelangle @LukasTy can you please look at this when you have time? I would like to confirm with you whether my approach for this is right. Thanks 🎉

@noraleonte noraleonte added the component: pickers This is the name of the generic UI component, not the React module! label May 23, 2023
@mui-bot
Copy link

mui-bot commented May 23, 2023

Localization writing tips ✍️

Seems you are updating localization 🌍 files.

Thank you for contributing to the localization! 🎉 To make your PR perfect, here is a list of elements to check: ✔️

  • Verify if the PR title respects the release format. Here are two examples (depending if you update or add a locale file)

    [l10n] Improve Swedish (sv-SE) locale
    [l10n] Add Danish (da-DK) locale

  • Update the documentation of supported locales by running yarn l10n
  • Clean files with yarn prettier.

Netlify deploy preview

Netlify deploy preview: https://deploy-preview-9095--material-ui-x.netlify.app/

Updated pages

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms -180.6 -7.9 -7.9 -76.86 63.651
Sort 100k rows ms 736.9 1,450.5 1,435 1,262.62 268.706
Select 100k rows ms 685.2 816.4 727.2 752.12 51.526
Deselect 100k rows ms 142 210.4 199.7 189.44 24.316

Generated by 🚫 dangerJS against 089d467

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.

Great start! 👍
I do think that the general direction is inline with what we were thinking about.
Refinement and integration with pickers would probably be the biggest problems with the most amount of different opinions. 🙈 🤷

 hook and event propagation

hover state and separate hook for endAdornment
@noraleonte noraleonte force-pushed the implement-clear-input-behavior branch from 557a597 to 64020ec Compare May 29, 2023 14:02
@noraleonte noraleonte requested a review from LukasTy May 29, 2023 14:13
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.

Nice progress! 👍
Some general comments:

  • it would be nice to play with styles so that the field width wouldn't change regardless of clearable flag and whether the button is visible or hidden.
  • should this feature be opt-in or opt-out as in Autocomplete (disableClearable)? cc @flaviendelangle @joserodolfofreitas

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 20, 2023
@github-actions
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 Jun 21, 2023
@noraleonte noraleonte marked this pull request as ready for review June 21, 2023 11:28
@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Sep 8, 2023
@github-actions
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 Sep 15, 2023
@noraleonte noraleonte requested a review from LukasTy September 21, 2023 11:37
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.

Great work! 💯 🚀 💙
It looks and feels ready for prime time and battle testing. 😱
While checking this PR I noticed a related problem that could easily be solved here as well.
Passing event empty slots object to BrowserDatePicker, BrowserDateRangePicker, and JoyDateRangePicker components causes the original TextField component to be used.
Could you please address this issue by swaping the order of {...props} spreading to be before the slots to avoid this issue? 🙈 🙏

@LukasTy LukasTy changed the title [pickers] Implement clearable behavior [pickers] Implement clearable field behavior Sep 21, 2023
@LukasTy LukasTy added new feature New feature or request feature: Keyboard editing Related to the pickers keyboard edition labels Sep 21, 2023
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.

Did a final detailed round of review, tested slots and slotProps usage as well as a11y behavior.
Here is a diff that fixes slots and slotProps propagation from picker level.
Also a fix to correctly handle className on IconButton to allow for additional className being passed via slotProps.clearButton.className.

diff --git a/packages/x-date-pickers-pro/src/internals/hooks/useEnrichedRangePickerFieldProps.ts b/packages/x-date-pickers-pro/src/internals/hooks/useEnrichedRangePickerFieldProps.ts
index d901ce8a6..d57cead66 100644
--- a/packages/x-date-pickers-pro/src/internals/hooks/useEnrichedRangePickerFieldProps.ts
+++ b/packages/x-date-pickers-pro/src/internals/hooks/useEnrichedRangePickerFieldProps.ts
@@ -321,12 +321,15 @@ const useSingleInputFieldSlotProps = <TDate, TView extends DateOrTimeViewWithMer
   const slots: ReturnType['slots'] = {
     ...fieldProps.slots,
     textField: pickerSlots?.textField,
+    clearButton: pickerSlots?.clearButton,
     clearIcon: pickerSlots?.clearIcon,
   };
 
   const slotProps: ReturnType['slotProps'] = {
     ...fieldProps.slotProps,
     textField: pickerSlotProps?.textField,
+    clearButton: pickerSlotProps?.clearButton,
+    clearIcon: pickerSlotProps?.clearIcon,
   };
 
   const enrichedFieldProps: ReturnType = {
diff --git a/packages/x-date-pickers/src/hooks/useClearableField.tsx b/packages/x-date-pickers/src/hooks/useClearableField.tsx
index a68448eda..9bcaad16c 100644
--- a/packages/x-date-pickers/src/hooks/useClearableField.tsx
+++ b/packages/x-date-pickers/src/hooks/useClearableField.tsx
@@ -45,14 +45,19 @@ export const useClearableField = <
   const localeText = useLocaleText();
 
   const IconButton = slots?.clearButton ?? MuiIconButton;
-  const iconButtonProps = useSlotProps({
-    elementType: MuiIconButton,
+  // The spread is here to avoid this bug mui/material-ui#34056
+  const { ownerState, ...iconButtonProps } = useSlotProps({
+    elementType: IconButton,
     externalSlotProps: slotProps?.clearButton,
     ownerState: {},
+    className: 'clearButton',
+    additionalProps: {
+      title: localeText.clearLabel,
+    },
   });
   const EndClearIcon = slots?.clearIcon ?? ClearIcon;
   const endClearIconProps = useSlotProps({
-    elementType: ClearIcon,
+    elementType: EndClearIcon,
     externalSlotProps: slotProps?.clearIcon,
     ownerState: {},
   });
@@ -65,12 +70,7 @@ export const useClearableField = <
           position="end"
           sx={{ marginRight: ForwardedInputProps?.endAdornment ? -1 : -1.5 }}
         >
-          <IconButton
-            aria-label={localeText.clearLabel}
-            {...iconButtonProps}
-            className="clearButton"
-            onClick={onClear}
-          >
+          <IconButton {...iconButtonProps} onClick={onClear}>
             <EndClearIcon fontSize="small" {...endClearIconProps} />
           </IconButton>
         </InputAdornment>

docs/data/date-pickers/custom-field/PickerWithJoyField.tsx Outdated Show resolved Hide resolved
docs/data/date-pickers/custom-field/PickerWithJoyField.tsx Outdated Show resolved Hide resolved
docs/data/date-pickers/custom-field/PickerWithJoyField.tsx Outdated Show resolved Hide resolved
Comment on lines 63 to 64
// Clear button label
clearLabel: 'Clear button',
Copy link
Member

@LukasTy LukasTy Sep 26, 2023

Choose a reason for hiding this comment

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

WDYT about going with a bit longer label and ditching the comment as the label key would seem self-explanatory. 🤔
WDYT about using Clear value label to be more expressive and avoid duplication on screen readers?
As it is, now screen reader reads this label as "Clear button, button".
IMHO, hearing "Clear value, button" would be a better UX. 🤔

P.S. I've tested only with title and screen readers seemed to be happy (MacOS Voice over and NVDA on Windows), but the "suggestion" on the interwebs seems to push towards providing both... Maybe... Especially in cases of anchor links, but for buttons without inner text... 🤷

Suggested change
// Clear button label
clearLabel: 'Clear button',
clearButtonLabel: 'Clear value',

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We already have a clearButtonLabel for the action bar. WDYT about clearIconButtonLabel?

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha.
Then maybe move it close to field labels and rename it to fieldClearButtonLabel or fieldClearLabel?
Both are good alternatives IMHO. Maybe the second one is even safer as it does not tie the label to the button implementation detail, but on the other hand - it's a little less clear. 🤷 🙈

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fieldClearLabel it is then 🎉

packages/x-date-pickers/src/hooks/useClearableField.tsx Outdated Show resolved Hide resolved
@LukasTy
Copy link
Member

LukasTy commented Sep 27, 2023

Forgot to initially take a look at the slots ?? components behavior.
We do not currently support it for these new slots. 🙈

Here is a diff to enable it on one of the components. The components and componentsSlots need to to be passed down the the hook from all usages.

diff --git a/packages/x-date-pickers-pro/src/SingleInputDateRangeField/SingleInputDateRangeField.tsx b/packages/x-date-pickers-pro/src/SingleInputDateRangeField/SingleInputDateRangeField.tsx
index cbde32ff7..940008ed2 100644
--- a/packages/x-date-pickers-pro/src/SingleInputDateRangeField/SingleInputDateRangeField.tsx
+++ b/packages/x-date-pickers-pro/src/SingleInputDateRangeField/SingleInputDateRangeField.tsx
@@ -69,6 +69,8 @@ const SingleInputDateRangeField = React.forwardRef(function SingleInputDateRange
     InputProps: fieldProps.InputProps,
     slots,
     slotProps,
+    components,
+    componentsProps,
   });
 
   return (
diff --git a/packages/x-date-pickers/src/hooks/useClearableField.tsx b/packages/x-date-pickers/src/hooks/useClearableField.tsx
index a68448eda..abfb93c18 100644
--- a/packages/x-date-pickers/src/hooks/useClearableField.tsx
+++ b/packages/x-date-pickers/src/hooks/useClearableField.tsx
@@ -22,6 +22,8 @@ type UseClearableFieldProps<
   onClear: React.MouseEventHandler<HTMLButtonElement>;
   slots?: { [K in keyof TFieldSlots as Uncapitalize<K & string>]: TFieldSlots[K] };
   slotProps?: TFieldSlotsComponentsProps;
+  components?: TFieldSlots;
+  componentsProps?: TFieldSlotsComponentsProps;
 };
 
 export const useClearableField = <
@@ -36,6 +38,8 @@ export const useClearableField = <
   onClear,
   slots,
   slotProps,
+  components,
+  componentsProps,
 }: UseClearableFieldProps<
   TFieldProps,
   TInputProps,
@@ -44,16 +48,16 @@ export const useClearableField = <
 >) => {
   const localeText = useLocaleText();
 
-  const IconButton = slots?.clearButton ?? MuiIconButton;
+  const IconButton = slots?.clearButton ?? components?.ClearButton ?? MuiIconButton;
   const iconButtonProps = useSlotProps({
     elementType: MuiIconButton,
-    externalSlotProps: slotProps?.clearButton,
+    externalSlotProps: slotProps?.clearButton ?? componentsProps?.clearButton,
     ownerState: {},
   });
-  const EndClearIcon = slots?.clearIcon ?? ClearIcon;
+  const EndClearIcon = slots?.clearIcon ?? components?.ClearIcon ?? ClearIcon;
   const endClearIconProps = useSlotProps({
     elementType: ClearIcon,
-    externalSlotProps: slotProps?.clearIcon,
+    externalSlotProps: slotProps?.clearIcon ?? componentsProps?.clearIcon,
     ownerState: {},
   });

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.

Looking great!
Massive effort on a very much anticipated feature. 👏
Left one final improvement to make the a11y experience better. 😉
I'd love for this to end up in this week's release. 🤞 🙏

packages/x-date-pickers/src/hooks/useClearableField.tsx Outdated Show resolved Hide resolved
@noraleonte noraleonte mentioned this pull request Sep 28, 2023
6 tasks
@noraleonte noraleonte merged commit 0453af4 into mui:master Sep 28, 2023
5 checks passed
@noraleonte noraleonte deleted the implement-clear-input-behavior branch September 28, 2023 11:19
michelengelen added a commit to michelengelen/mui-x that referenced this pull request Sep 28, 2023
michelengelen added a commit to michelengelen/mui-x that referenced this pull request Sep 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: pickers This is the name of the generic UI component, not the React module! feature: Keyboard editing Related to the pickers keyboard edition new feature New feature or request
Projects
None yet
5 participants