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

#25730: TimePicker Default Value Fix, Controllable Usage, Example Updates #26482

Merged

Conversation

CheerfulSatchel
Copy link
Contributor

@CheerfulSatchel CheerfulSatchel commented Jan 24, 2023

Greetings, reviewer! This PR addresses issue #25730.

Initial Issue

The defaultValue prop represents the baseline date for the TimePicker time. Currently, the defaultValue prop is stored in a useConst call, so if users wished to update TimePicker's defaultValue (i.e. from a DatePicker) the TimePicker would not reflect the updated defaultValue. As a result, if the user were to select a new time in the TimePicker, the onChange date object reflects the correct time (hours/minutes/seconds) but would only use the very first defaultValue prop supplied.

This issue makes the date object supplied in the onChange callback function erroneous in cases where users want to change defaultValue.

Addressing the Bigger Issue

The pre-existing TimePicker does not do a great job handling uncontrolled use-cases, and doesn't even support controlled use-cases. This led to bugs such as having a fixed TimePicker defaultValue and inability for users to pass in their own date objects to control the current time. Ultimately, these issues render the TimePicker unusable in most cases.

Changes

Uncontrolled use case

defaultValue and onChange are commonly used to get the value of uncontrolled controls. Originally, the defaultValue took the role of being the "baseline" date to calculate all dropdown options. Really, defaultValue should be the initial value selected in the control and that is its new role now. Now, a new prop dateAnchor takes the role as the "baseline" date to calculate all dropdown date options.

Controlled use case

value and onChange are commonly used for component callers to manually set and handle the value of controlled controls. Therefore, I have introduced a new prop value for this purpose. You can find sample usage of this in TimePicker.Controlled.Example.tsx and TimePicker.DateTimePicker.Example.tsx.

Demo

All the examples for TimePicker have been pulled into their own respective files. These examples are Basic.Example, Controlled.Example, CustomTimeStrings.Example, and DateTimePicker.Example.

Also, the samples look cooler now.

New basic example

image

New DateTimePicker example

image

Related Issue(s)

@fabricteam
Copy link
Collaborator

fabricteam commented Jan 24, 2023

📊 Bundle size report

🤖 This report was generated against ee977e20d359922798949a4f18af7a036ca36ca3

@fabricteam
Copy link
Collaborator

fabricteam commented Jan 24, 2023

Perf Analysis (@fluentui/react)

Scenario Render type Master Ticks PR Ticks Iterations Status
ThemeProvider mount 1370 1355 5000 Possible regression
All results

Scenario Render type Master Ticks PR Ticks Iterations Status
BaseButton mount 1003 993 5000
Breadcrumb mount 2747 2760 1000
Checkbox mount 2643 2618 5000
CheckboxBase mount 2268 2267 5000
ChoiceGroup mount 4552 4530 5000
ComboBox mount 1072 1095 1000
CommandBar mount 9954 10142 1000
ContextualMenu mount 22142 22504 1000
DefaultButton mount 1176 1212 5000
DetailsRow mount 3535 3495 5000
DetailsRowFast mount 3467 3497 5000
DetailsRowNoStyles mount 3206 3334 5000
Dialog mount 4154 4076 1000
DocumentCardTitle mount 377 398 1000
Dropdown mount 3037 3063 5000
FocusTrapZone mount 1822 1863 5000
FocusZone mount 1763 1726 5000
GroupedList mount 58824 69451 2
GroupedList virtual-rerender 29016 29075 2
GroupedList virtual-rerender-with-unmount 87986 88853 2
GroupedListV2 mount 396 380 2
GroupedListV2 virtual-rerender 363 375 2
GroupedListV2 virtual-rerender-with-unmount 379 388 2
IconButton mount 1745 1680 5000
Label mount 547 557 5000
Layer mount 4352 4389 5000
Link mount 631 657 5000
MenuButton mount 1492 1484 5000
MessageBar mount 33598 33803 5000
Nav mount 3088 3124 1000
OverflowSet mount 1194 1212 5000
Panel mount 2709 2767 1000
Persona mount 1229 1171 1000
Pivot mount 1400 1443 1000
PrimaryButton mount 1337 1321 5000
Rating mount 6923 7039 5000
SearchBox mount 1391 1374 5000
Shimmer mount 2874 2794 5000
Slider mount 2074 2068 5000
SpinButton mount 4512 4510 5000
Spinner mount 643 619 5000
SplitButton mount 2921 2877 5000
Stack mount 631 657 5000
StackWithIntrinsicChildren mount 1384 1396 5000
StackWithTextChildren mount 3927 3911 5000
SwatchColorPicker mount 9666 9692 5000
TagPicker mount 2417 2402 5000
Text mount 598 582 5000
TextField mount 1455 1452 5000
ThemeProvider mount 1370 1355 5000 Possible regression
ThemeProvider virtual-rerender 926 936 5000
ThemeProvider virtual-rerender-with-unmount 2101 2073 5000
Toggle mount 955 958 5000
buttonNative mount 333 356 5000

@layershifter
Copy link
Member

layershifter commented Jan 24, 2023

The first thing done was to rename props to be more descriptive, namely
defaultValue -> currentDate
onChange -> onTimeChange

I am not a reviewer of this area, but props rename is a breaking change until old ones are preserved. More or less, it means that the PR cannot be merged with these changes.

@CheerfulSatchel CheerfulSatchel changed the title #25730: TimePicker Default Value Fix #25730: TimePicker Default Value Fix, Controllable Usage, Example Updates Feb 27, 2023
@CheerfulSatchel CheerfulSatchel marked this pull request as ready for review February 28, 2023 18:36
@CheerfulSatchel CheerfulSatchel requested review from a team as code owners February 28, 2023 18:36
Copy link
Contributor

@spmonahan spmonahan left a comment

Choose a reason for hiding this comment

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

Looks like the build is failing because you have API updates. You'll need to generate a new API snapshot with yarn lage build --to @fluentui/react from the repo root.

Copy link
Contributor

@behowell behowell left a comment

Choose a reason for hiding this comment

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

A few more minor comments, but please see my reply about the error message callback: #26482 (comment)

Once the error message callback is updated, this should be good to go. Thanks for your patience with the review!

@fabricteam
Copy link
Collaborator

fabricteam commented Apr 24, 2023

🕵 fluentuiv8 No visual regressions between this PR and main

Copy link
Contributor

@sopranopillow sopranopillow left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@behowell behowell 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!

@behowell behowell merged commit 2d93a25 into microsoft:master Apr 25, 2023
marcosmoura added a commit to marcosmoura/fluentui that referenced this pull request Apr 25, 2023
* master:
  applying package updates
  microsoft#25730: TimePicker Default Value Fix, Controllable Usage, Example Updates (microsoft#26482)
  docs(react-datepicker-compat): Add description to README.md (microsoft#27677)
  Updated to use single hook selector (microsoft#27491)
  fix: Make border around Avatar's badge transparent using a mask (microsoft#27527)
  Disable focus on non-interactive elements (microsoft#27580)
  feat(react-drawer): implement "prevent close on click outside" feature (microsoft#27551)
marcosmoura added a commit to marcosmoura/fluentui that referenced this pull request Apr 25, 2023
* master:
  applying package updates
  microsoft#25730: TimePicker Default Value Fix, Controllable Usage, Example Updates (microsoft#26482)
  docs(react-datepicker-compat): Add description to README.md (microsoft#27677)
  Updated to use single hook selector (microsoft#27491)
  fix: Make border around Avatar's badge transparent using a mask (microsoft#27527)
  Disable focus on non-interactive elements (microsoft#27580)
  feat(react-drawer): implement "prevent close on click outside" feature (microsoft#27551)
marcosmoura added a commit to marcosmoura/fluentui that referenced this pull request Apr 25, 2023
* feat/drawer-components: (81 commits)
  docs: remove TODO marks
  fix: remove conflict code leftover
  applying package updates
  microsoft#25730: TimePicker Default Value Fix, Controllable Usage, Example Updates (microsoft#26482)
  docs(react-datepicker-compat): Add description to README.md (microsoft#27677)
  Updated to use single hook selector (microsoft#27491)
  fix: Make border around Avatar's badge transparent using a mask (microsoft#27527)
  Disable focus on non-interactive elements (microsoft#27580)
  feat(react-drawer): implement "prevent close on click outside" feature (microsoft#27551)
  fix(react-datepicker-compat): Make onValidationError onValidationResult so the error is updated when there's no longer an error (microsoft#27655)
  Fix `@fluentui/react-portal-compat`: to be compatible with React 18 (microsoft#27577)
  chore: fix versions of @fluentui/react-datepicker-compat (microsoft#27666)
  applying package updates
  applying package updates
  Make line chart screen reader accessible (microsoft#27632)
  fix(react-examples): Improve keyboard navigation in ContextualMenu.CustomMenuList (microsoft#25172)
  docs(react-textarea): Update examples to use Field (microsoft#27644)
  bugfix(react-dialog): `DialogTitle` root as `h2` by default (microsoft#27555)
  applying package updates
  chore: restructure stories, add separate category for flat tree (microsoft#27586)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Specifying defaultValue doesn't modify the timePicker
9 participants