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

Migrate Calendar components to function components #12928

Closed
wants to merge 53 commits into from

Conversation

MLoughry
Copy link
Contributor

@MLoughry MLoughry commented Apr 29, 2020

Pull request checklist

  • Addresses an existing issue: Fluent UI v8 #12770
  • Include a change request file using $ yarn change

Description of changes

Migrate all Calendar components to function components.

Also added a utility method for function components to apply default properties, as a replacement for static defaultProps

Focus areas to test

(optional)

Microsoft Reviewers: Open in CodeFlow

MLoughry and others added 30 commits April 9, 2020 17:52
@msft-github-bot
Copy link
Contributor

msft-github-bot commented Apr 29, 2020

Perf Analysis

No significant results to display.

All results

Scenario Master Ticks PR Ticks Iterations Status
BaseButton 882 830 5000
Checkbox 1545 1730 5000
CheckboxBase 1257 1310 5000
CheckboxNext 1578 1529 5000
ChoiceGroup 4989 4987 5000
ComboBox 858 891 1000
CommandBar 7097 6963 1000
ContextualMenu 13087 15486 1000
DefaultButton 1084 1124 5000
DetailsRow 3478 3400 5000
DetailsRow (fast icons) 3502 3496 5000
DetailsRow without styles 3127 3229 5000
Dialog 1418 1465 1000
DocumentCardTitle with truncation 1513 1640 1000
Dropdown 2469 2375 5000
FocusZone 1545 1616 5000
IconButton 1695 1734 5000
Label 280 310 5000
Link 446 446 5000
LinkNext 452 442 5000
MenuButton 1403 1371 5000
Nav 3048 3065 1000
Panel 1416 1405 1000
Persona 831 828 1000
Pivot 1312 1285 1000
PrimaryButton 1255 1217 5000
SearchBox 1282 1258 5000
Slider 1428 1498 5000
Spinner 352 393 5000
SplitButton 3001 3042 5000
Stack 467 467 5000
Stack with Intrinsic children 1105 1127 5000
Stack with Text children 4430 4367 5000
TagPicker 2665 2709 5000
Text 393 399 5000
TextField 1336 1335 5000
Toggle 849 855 5000
button 69 71 5000

Perf Analysis (Fluent)

Perf comparison
Status Scenario Fluent TPI Fabric TPI Ratio Iterations Ticks
🔧 Avatar.Fluent 0.48 0.47 1.02:1 2000 951
🦄 Button.Fluent 0.11 0.2 0.55:1 5000 548
🔧 Checkbox.Fluent 0.64 0.33 1.94:1 1000 641
🔧 Dialog.Fluent 0.36 0.19 1.89:1 5000 1823
🔧 Dropdown.Fluent 3.24 0.45 7.2:1 1000 3235
🔧 Icon.Fluent 0.15 0.05 3:1 5000 729
🎯 Image.Fluent 0.08 0.1 0.8:1 5000 389
🔧 Slider.Fluent 1.33 0.36 3.69:1 1000 1332
🔧 Text.Fluent 0.07 0.02 3.5:1 5000 353
🦄 Tooltip.Fluent 0.09 16.71 0.01:1 5000 470

🔧 Needs work     🎯 On target     🦄 Amazing

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
RefMinimalPerf.default 205 184 1.11:1
VideoMinimalPerf.default 644 592 1.09:1
LabelMinimalPerf.default 425 392 1.08:1
RadioGroupMinimalPerf.default 611 571 1.07:1
StatusMinimalPerf.default 691 648 1.07:1
TreeWith60ListItems.default 221 206 1.07:1
FlexMinimalPerf.default 304 286 1.06:1
AvatarMinimalPerf.default 521 496 1.05:1
BoxMinimalPerf.default 344 329 1.05:1
ChatDuplicateMessagesPerf.default 410 389 1.05:1
ButtonSlotsPerf.default 628 605 1.04:1
DialogMinimalPerf.default 1804 1739 1.04:1
DropdownManyItemsPerf.default 1328 1282 1.04:1
ImageMinimalPerf.default 391 377 1.04:1
InputMinimalPerf.default 978 939 1.04:1
PortalMinimalPerf.default 327 315 1.04:1
IconMinimalPerf.default 703 674 1.04:1
ButtonMinimalPerf.default 164 159 1.03:1
CarouselMinimalPerf.default 476 462 1.03:1
ChatWithPopoverPerf.default 518 504 1.03:1
DividerMinimalPerf.default 345 336 1.03:1
ListCommonPerf.default 1028 998 1.03:1
TextAreaMinimalPerf.default 500 484 1.03:1
TooltipMinimalPerf.default 718 694 1.03:1
Tooltip.Fluent 470 457 1.03:1
HierarchicalTreeMinimalPerf.default 1034 1015 1.02:1
LayoutMinimalPerf.default 585 573 1.02:1
PopupMinimalPerf.default 250 245 1.02:1
ProviderMergeThemesPerf.default 1792 1755 1.02:1
TreeMinimalPerf.default 1260 1237 1.02:1
Dropdown.Fluent 3235 3161 1.02:1
Icon.Fluent 729 718 1.02:1
Image.Fluent 389 380 1.02:1
AccordionMinimalPerf.default 138 136 1.01:1
AlertMinimalPerf.default 288 286 1.01:1
AttachmentSlotsPerf.default 1138 1123 1.01:1
CardMinimalPerf.default 586 579 1.01:1
HeaderMinimalPerf.default 501 495 1.01:1
MenuButtonMinimalPerf.default 1615 1601 1.01:1
SliderMinimalPerf.default 1316 1306 1.01:1
SplitButtonMinimalPerf.default 3412 3383 1.01:1
ToolbarMinimalPerf.default 847 838 1.01:1
Checkbox.Fluent 641 632 1.01:1
ChatMinimalPerf.default 632 632 1:1
CheckboxMinimalPerf.default 2807 2812 1:1
ListWith60ListItems.default 1165 1161 1:1
LoaderMinimalPerf.default 745 742 1:1
ReactionMinimalPerf.default 382 383 1:1
TableMinimalPerf.default 388 389 1:1
Avatar.Fluent 951 953 1:1
Button.Fluent 548 550 1:1
Dialog.Fluent 1823 1823 1:1
AnimationMinimalPerf.default 643 652 0.99:1
ListMinimalPerf.default 475 482 0.99:1
ProviderMinimalPerf.default 645 654 0.99:1
CustomToolbarPrototype.default 3446 3496 0.99:1
FormMinimalPerf.default 417 424 0.98:1
GridMinimalPerf.default 697 713 0.98:1
HeaderSlotsPerf.default 1516 1542 0.98:1
SegmentMinimalPerf.default 339 345 0.98:1
Slider.Fluent 1332 1366 0.98:1
EmbedMinimalPerf.default 1991 2060 0.97:1
ItemLayoutMinimalPerf.default 1703 1750 0.97:1
TextMinimalPerf.default 338 350 0.97:1
Text.Fluent 353 363 0.97:1
DropdownMinimalPerf.default 3156 3278 0.96:1
ListNestedPerf.default 923 958 0.96:1
MenuMinimalPerf.default 661 692 0.96:1
AttachmentMinimalPerf.default 141 149 0.95:1

@MLoughry
Copy link
Contributor Author

MLoughry commented May 4, 2020

@dzearing @ecraig12345 Not sure of the advice concerning this build error:

@fluentui/react-next: [8:02:30 PM] x src/index.ts:18:1 - error TS2308: Module './Calendar' has already exported a member named 'DateRangeType'. Consider explicitly re-exporting to resolve the ambiguity.
@fluentui/react-next: 18 export * from './DatePicker';

Basically, since certain types are being re-exported from multiple components, and those types are now technically different when they're from different source files (OUFR vs react-next), we get this error.

Should we just copy OUFR over to react-next wholesale?

@ecraig12345
Copy link
Member

@MLoughry Would you mind splitting out the default props utility into a different PR? It will be easier to get that specific part reviewed and merged.

@lorejoh12 Could you take a look at the other parts of this?

@MLoughry
Copy link
Contributor Author

@MLoughry Would you mind splitting out the default props utility into a different PR? It will be easier to get that specific part reviewed and merged.

I created #13152 to split that off.

@ecraig12345
Copy link
Member

ecraig12345 commented May 14, 2020

Basically, since certain types are being re-exported from multiple components, and those types are now technically different when they're from different source files (OUFR vs react-next), we get this error.

Should we just copy OUFR over to react-next wholesale?

The issue with exporting stuff from multiple places is probably something we should fix as part of version 8--apparently it's already causing problems for people in some cases (see #12350).

In some cases it will be a bit tricky to determine the single "right" place things should be exported from, but it seems pretty obvious that DatePicker shouldn't be exporting Calendar stuff. So maybe for this specific case, you could change react-next/src/DatePicker to manually export only DatePicker stuff? (copying oufr/src/components/DatePicker/index minus the Calendar line)

@msft-github-bot
Copy link
Contributor

@joschect

Gentle ping that this issue needs attention.

@MLoughry
Copy link
Contributor Author

MLoughry commented Jun 9, 2020

@ecraig12345 @joschect Any further concerns blocking sign-off?

@dzearing
Copy link
Member

I've started a conversation with the code owners here; not sure about this one.

@dzearing
Copy link
Member

dzearing commented Jun 18, 2020

Ok after talking with @lorejoh12 , let's do this:

  1. Let's NOT base the update on the old Calendar. Instead, we should update the Calendar in date-time.
  2. Let's export the date-time package.

There are numerous issues which have been resolved in date-time, but not in the existing v7 Calendar component due to breaking changes. It would be wasteful to convert it and then replace it later.

This also applies to DatePicker.

Sorry for the trouble @MLoughry!

@MLoughry MLoughry closed this Jun 18, 2020
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.

5 participants