bugfix:removed useConstant hook for initial value in timepicker #25913

HarminderSethi · 2022-12-07T04:34:41Z

Previous Behavior

TimePicker when used with DatePicker return the current date with the selected time from TimePicker instead of DatePicker date. This was happeing due to useConst hook which set the current date value at initial render and does not update if the value change

New Behavior

Removed the useConst hook for initial value which will help the TimePicker date sync with DatePicker selected date

Related Issue(s)

size-auditor · 2022-12-07T04:48:14Z

Asset size changes

Project	Bundle	Baseline Size	New Size	Difference
office-ui-fabric-react	fluentui-react-TimePicker	226.267 kB	226.264 kB	-3 bytes

Over Tolerance (1024 B) Over Baseline Below Baseline New Removed 1 kB = 1000 B

Baseline commit: 134bbf565b13a0375a8db4ac022737d45e46c425 (build)

codesandbox-ci · 2022-12-07T04:49:38Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 08c7ea7:

Sandbox	Source
@fluentui/react 8 starter	Configuration
@fluentui/react-components 9 starter	Configuration

fabricteam · 2022-12-07T04:52:18Z

📊 Bundle size report

_{🤖 This report was generated against 134bbf565b13a0375a8db4ac022737d45e46c425}

fabricteam · 2022-12-07T04:58:55Z

🕵 fluentuiv8 No visual regressions between this PR and main

fabricteam · 2022-12-07T05:05:47Z

Perf Analysis (`@fluentui/react`)

Scenario	Render type	Master Ticks	PR Ticks	Iterations	Status
GroupedList	mount	2589	2961	2	Possible regression

All results

Scenario	Render type	Master Ticks	PR Ticks	Iterations	Status
BaseButton	mount	1575	1572	5000
Breadcrumb	mount	3692	3734	1000
Checkbox	mount	3393	3623	5000
CheckboxBase	mount	3122	3157	5000
ChoiceGroup	mount	5819	5932	5000
ComboBox	mount	1566	1616	1000
CommandBar	mount	12106	12080	1000
ContextualMenu	mount	13983	14046	1000
DefaultButton	mount	1806	1806	5000
DetailsRow	mount	4658	4754	5000
DetailsRowFast	mount	4702	4673	5000
DetailsRowNoStyles	mount	4526	4516	5000
Dialog	mount	3949	3910	1000
DocumentCardTitle	mount	732	765	1000
Dropdown	mount	4280	4161	5000
FocusTrapZone	mount	2704	2496	5000
FocusZone	mount	2442	2480	5000
GroupedList	mount	2589	2961	2	Possible regression
GroupedList	virtual-rerender	1497	1521	2
GroupedList	virtual-rerender-with-unmount	2306	2137	2
GroupedListV2	mount	736	728	2
GroupedListV2	virtual-rerender	706	698	2
GroupedListV2	virtual-rerender-with-unmount	756	732	2
IconButton	mount	2520	2582	5000
Label	mount	917	919	5000
Layer	mount	5504	5629	5000
Link	mount	1171	1074	5000
MenuButton	mount	2298	2168	5000
MessageBar	mount	2951	2913	5000
Nav	mount	4338	4256	1000
OverflowSet	mount	1706	1726	5000
Panel	mount	3214	3187	1000
Persona	mount	1696	1709	1000
Pivot	mount	2170	2113	1000
PrimaryButton	mount	1957	1998	5000
Rating	mount	9237	9328	5000
SearchBox	mount	2001	2036	5000
Shimmer	mount	3774	3851	5000
Slider	mount	2734	2677	5000
SpinButton	mount	6076	5970	5000
Spinner	mount	1033	1022	5000
SplitButton	mount	4014	4048	5000
Stack	mount	1064	1091	5000
StackWithIntrinsicChildren	mount	3338	3267	5000
StackWithTextChildren	mount	6594	6544	5000
SwatchColorPicker	mount	13438	13627	5000
TagPicker	mount	3361	3396	5000
TeachingBubble	mount	109054	109075	5000
Text	mount	1034	1014	5000
TextField	mount	2083	2050	5000
ThemeProvider	mount	1913	1955	5000
ThemeProvider	virtual-rerender	1360	1334	5000
ThemeProvider	virtual-rerender-with-unmount	2723	2716	5000
Toggle	mount	1409	1410	5000
buttonNative	mount	700	688	5000

spmonahan · 2022-12-08T00:59:52Z

packages/react/src/components/TimePicker/TimePicker.tsx

  const optionsCount = getDropdownOptionsCount(increments, timeRange);

-  const initialValue = useConst(defaultValue || new Date());
+  const initialValue = defaultValue || new Date();


I'm not sure this is the right change to make, though I'm not very familiar with the TimePicker code so I could be wrong 🙂

defaultValue is typically used for uncontrolled React components and removing useConst() won't produce the correct behavior for an uncontrolled component (nor does leaving it in place).

My initial thought is that we should be using useControllableValue here but that may be a breaking change.

Change you provide a Codepen example of the issue as it stands today so I can understand the problem you're solving better?

@spmonahan I tried replicating the issue on Sandbox but it is working fine. I am facing the issue with SPFx and using TimePicker with DatePicker. First I select the date from datepicker(not today date) and then select the time. the selected date from datepicker is passed to timepicker via defaultValue property. but as soon as I select the value from timpicker it returns the todays date with the selected time. Ideally it should return the date picker date with the time selected. when I debug the control I found that initial value is set only once when the control intialized due to useConst (because at that time the defaultValue was null (as datepicker did not have value selected). If useConst is removed then the initial value will based on either the latest default value or current date.

And you are right, useControllableValue hook should be used here.

Sounds like you have it in hand. When you update this PR with useControllableValue would you also add a test cases for controlled and uncontrolled usages of TimePicker?

@spmonahan I do not have expertise in writing the test cases but I tried after looking into the existing test cases and found that changed value of the control gets updated either as the value attribute of the html element or any child element but in case of TimePicker only updated/selected time gets available in the input HTML element associated with the TimePicker. tried some other ways but no luck.. may be in coming future I will be able to handle the test cases as well.

I have made the changes related to the useControllableValue hook.

@spmonahan could you please review the changes?

… update the initialValue for controlled TimePicker

HarminderSethi · 2022-12-27T15:46:05Z

@microsoft-github-policy-service agree [company="default"]

HarminderSethi · 2022-12-27T15:47:09Z

@microsoft-github-policy-service agree

spmonahan · 2023-01-03T17:58:14Z

packages/react/src/components/TimePicker/TimePicker.tsx


-  const initialValue = useConst(defaultValue || new Date());
-  const baseDate: Date = React.useMemo(() => generateBaseDate(increments, timeRange, initialValue), [
+  const [initialValue] = useControllableValue(defaultValue, new Date());


I don't think this is the correct change. The first argument to useControllableValue is meant to be the controlled value. defaultValue is React's way of handling uncontrolled values.

I think this change needs to update TimePicker to support both controlled and uncontrolled usage. However, I'm not 100% certain how that should be handled as I believe TimePicker is used more like a ComboBox so adding a controlled value may not make sense.

@CheerfulSatchel, do you have any ideas on how to proceed here?

But defaultValue will be passed as props. If it is used with datepicker then default value will hold the value selected in datepicker.

I think in other controls also this scenario is handled in same way.

@spmonahan @CheerfulSatchel have we reached on any consensus for this issue?

CheerfulSatchel · 2023-01-24T18:08:16Z

Hey @HarminderSethi , sorry for the wait. I really appreciate your changes and the ongoing conversation. Might I add on to the work you have done, I had created this PR #26482 which addresses the same bug but also goes further and adds a sample DatePicker/TimePicker combination. Please check out this PR and let me know your thoughts.

msft-fluent-ui-bot · 2023-06-24T00:00:03Z

Because this pull request has not had activity for over 150 days, we're automatically closing it for house-keeping purposes.

The pull request will still be available for reference. If it's still relevant to merge at some point, you can reopen or make a new version based on the latest code.

bugfix:removed useConstant hook for initial value in timepicker

941068e

HarminderSethi requested review from a team as code owners December 7, 2022 04:34

msft-fluent-ui-bot added the Component: TimePicker label Dec 7, 2022

msft-fluent-ui-bot assigned khmakoto Dec 7, 2022

spmonahan reviewed Dec 8, 2022

View reviewed changes

useConst replaced with useControllableValue hook as useConst does not…

08c7ea7

… update the initialValue for controlled TimePicker

CheerfulSatchel approved these changes Dec 16, 2022

View reviewed changes

spmonahan reviewed Jan 3, 2023

View reviewed changes

msft-fluent-ui-bot added the Resolution: Soft Close Soft closing inactive issues over a certain period label Jun 24, 2023

msft-fluent-ui-bot closed this Jun 24, 2023

bugfix:removed useConstant hook for initial value in timepicker #25913

bugfix:removed useConstant hook for initial value in timepicker #25913

Uh oh!

Conversation

HarminderSethi commented Dec 7, 2022

Previous Behavior

New Behavior

Related Issue(s)

Uh oh!

size-auditor bot commented Dec 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Asset size changes

Uh oh!

codesandbox-ci bot commented Dec 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fabricteam commented Dec 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📊 Bundle size report

Uh oh!

fabricteam commented Dec 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🕵 fluentuiv8 No visual regressions between this PR and main

Uh oh!

fabricteam commented Dec 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HarminderSethi commented Dec 27, 2022

Uh oh!

HarminderSethi commented Dec 27, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CheerfulSatchel commented Jan 24, 2023

Uh oh!

msft-fluent-ui-bot commented Jun 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

size-auditor bot commented Dec 7, 2022 •

edited

Loading

codesandbox-ci bot commented Dec 7, 2022 •

edited

Loading

fabricteam commented Dec 7, 2022 •

edited

Loading

fabricteam commented Dec 7, 2022 •

edited

Loading

fabricteam commented Dec 7, 2022 •

edited

Loading