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.

Sign up for GitHub

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

Jump to bottom

List: change type of version prop to any #18257

Merged
merged 4 commits into from
May 25, 2021
Merged

List: change type of version prop to any #18257

merged 4 commits into from
May 25, 2021

Conversation

hetanthakkar
Copy link
Contributor

@hetanthakkar hetanthakkar commented May 19, 2021 •
edited by ecraig12345

Pull request checklist

Description of changes

change in onRenderCell prop will now re-render the list

Changed type of version prop to any to indicate that any value can be used to force a re-render.

Focus areas to test

(optional)

@hetanthakkar
Copy link
Contributor Author

@ecraig12345 Can you please review!

@hetanthakkar
Copy link
Contributor Author

Can you please tell me how can I fix the failed build

@codesandbox-ci
Copy link

codesandbox-ci bot commented May 19, 2021 •
edited

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 3c3cbca:

Sandbox Source
Fluent UI Button Configuration
codesandbox-react-template Configuration
codesandbox-react-northstar-template Configuration

@size-auditor
Copy link

size-auditor bot commented May 19, 2021 •
edited

Asset size changes

Size Auditor did not detect a change in bundle size for any component!

Baseline commit: fc8a879920a6df5538b96cf381c743a17b64df00 (build)

@fabricteam
Copy link
Collaborator

fabricteam commented May 19, 2021 •
edited

Perf Analysis (@fluentui/react)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 923 964 5000
BaseButton mount 1032 1029 5000
Breadcrumb mount 2809 2796 1000
ButtonNext mount 547 584 5000
Checkbox mount 1748 1720 5000
CheckboxBase mount 1432 1496 5000
ChoiceGroup mount 5357 5282 5000
ComboBox mount 1056 1045 1000
CommandBar mount 10862 10887 1000
ContextualMenu mount 6659 6669 1000
DefaultButton mount 1260 1271 5000
DetailsRow mount 4128 4093 5000
DetailsRowFast mount 4126 4160 5000
DetailsRowNoStyles mount 3913 3875 5000
Dialog mount 2329 2355 1000
DocumentCardTitle mount 173 165 1000
Dropdown mount 3643 3619 5000
FocusTrapZone mount 1870 1971 5000
FocusZone mount 1934 1896 5000
IconButton mount 1918 1966 5000
Label mount 381 361 5000
Layer mount 1976 1961 5000
Link mount 503 527 5000
MakeStyles mount 1925 1881 50000
MenuButton mount 1641 1649 5000
MessageBar mount 2175 2205 5000
Nav mount 3645 3681 1000
OverflowSet mount 1139 1115 5000
Panel mount 2193 2297 1000
Persona mount 892 906 1000
Pivot mount 1549 1560 1000
PrimaryButton mount 1427 1446 5000
Rating mount 8660 8597 5000
SearchBox mount 1482 1508 5000
Shimmer mount 2837 2857 5000
Slider mount 2180 2173 5000
SpinButton mount 5487 5536 5000
Spinner mount 457 447 5000
SplitButton mount 3565 3499 5000
Stack mount 568 556 5000
StackWithIntrinsicChildren mount 1696 1735 5000
StackWithTextChildren mount 5136 5059 5000
SwatchColorPicker mount 11173 11247 5000
Tabs mount 1525 1578 1000
TagPicker mount 2665 2724 5000
TeachingBubble mount 12676 12644 5000
Text mount 467 502 5000
TextField mount 1520 1530 5000
ThemeProvider mount 1274 1292 5000
ThemeProvider virtual-rerender 635 662 5000
ThemeProviderNext mount 6872 6899 5000
Toggle mount 883 899 5000
buttonNative mount 122 118 5000

Perf Analysis (@fluentui/react-northstar)

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
TextMinimalPerf.default 419 377 1.11:1
BoxMinimalPerf.default 415 380 1.09:1
DialogMinimalPerf.default 852 800 1.07:1
DividerMinimalPerf.default 417 389 1.07:1
ListMinimalPerf.default 586 548 1.07:1
SegmentMinimalPerf.default 406 381 1.07:1
IconMinimalPerf.default 726 678 1.07:1
LabelMinimalPerf.default 467 439 1.06:1
LoaderMinimalPerf.default 799 764 1.05:1
PortalMinimalPerf.default 173 165 1.05:1
AnimationMinimalPerf.default 453 434 1.04:1
ButtonMinimalPerf.default 199 191 1.04:1
ChatDuplicateMessagesPerf.default 328 315 1.04:1
HeaderMinimalPerf.default 423 407 1.04:1
LayoutMinimalPerf.default 425 409 1.04:1
ListWith60ListItems.default 735 705 1.04:1
ReactionMinimalPerf.default 443 426 1.04:1
RefMinimalPerf.default 249 240 1.04:1
SkeletonMinimalPerf.default 408 393 1.04:1
CustomToolbarPrototype.default 4167 4021 1.04:1
ToolbarMinimalPerf.default 1044 1006 1.04:1
AccordionMinimalPerf.default 174 169 1.03:1
AlertMinimalPerf.default 317 307 1.03:1
AttachmentMinimalPerf.default 175 170 1.03:1
ButtonSlotsPerf.default 628 609 1.03:1
InputMinimalPerf.default 1362 1317 1.03:1
ListCommonPerf.default 721 703 1.03:1
StatusMinimalPerf.default 771 745 1.03:1
TableManyItemsPerf.default 2214 2148 1.03:1
TableMinimalPerf.default 461 448 1.03:1
AttachmentSlotsPerf.default 1305 1282 1.02:1
AvatarMinimalPerf.default 220 216 1.02:1
ChatWithPopoverPerf.default 398 391 1.02:1
ItemLayoutMinimalPerf.default 1441 1417 1.02:1
MenuMinimalPerf.default 954 938 1.02:1
RadioGroupMinimalPerf.default 497 485 1.02:1
TooltipMinimalPerf.default 1080 1058 1.02:1
TreeMinimalPerf.default 877 857 1.02:1
TreeWith60ListItems.default 188 184 1.02:1
ButtonOverridesMissPerf.default 1831 1805 1.01:1
DatepickerMinimalPerf.default 5960 5900 1.01:1
DropdownManyItemsPerf.default 771 762 1.01:1
FlexMinimalPerf.default 320 317 1.01:1
HeaderSlotsPerf.default 881 874 1.01:1
RosterPerf.default 1318 1303 1.01:1
ProviderMinimalPerf.default 1070 1063 1.01:1
SplitButtonMinimalPerf.default 4142 4107 1.01:1
CardMinimalPerf.default 628 627 1:1
CarouselMinimalPerf.default 520 520 1:1
CheckboxMinimalPerf.default 2957 2947 1:1
DropdownMinimalPerf.default 3284 3272 1:1
EmbedMinimalPerf.default 4450 4470 1:1
MenuButtonMinimalPerf.default 1759 1763 1:1
PopupMinimalPerf.default 615 614 1:1
ChatMinimalPerf.default 690 699 0.99:1
ImageMinimalPerf.default 435 439 0.99:1
ProviderMergeThemesPerf.default 1726 1738 0.99:1
GridMinimalPerf.default 404 412 0.98:1
ListNestedPerf.default 608 622 0.98:1
SliderMinimalPerf.default 1678 1710 0.98:1
VideoMinimalPerf.default 696 709 0.98:1
TextAreaMinimalPerf.default 549 573 0.96:1
FormMinimalPerf.default 451 508 0.89:1

ecraig12345
ecraig12345 requested changes May 19, 2021
View reviewed changes
Copy link
Member

@ecraig12345 ecraig12345 left a comment

Choose a reason for hiding this comment

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

List is a very old, touchy component, and adding another condition which causes it to re-render could have a lot of negative perf implications. This is especially true if it's being used from within a function component where it's easy to accidentally re-create callback functions on every render.

I'm interested to hear what @ThomasMichon thinks as well, but my initial opinion is that it would be safer to leave the code as-is, and let the component consumer use the version prop to force re-renders.

(We really appreciate your contributions, but this is another example of why you need to comment on the issue to confirm that it's ready to go and the general idea of the proposed fix is okay before starting to work on something.)

@ecraig12345
Copy link
Member

@hetanthakkar1 Alternatively, if you'd like to update the type of IListProps.version to be any as described here, I'd be okay with that.
#18216 (comment)

@hetanthakkar
Copy link
Contributor Author

@ecraig12345 I get it. Sorry for not commenting on the issue before making PR

@hetanthakkar
Copy link
Contributor Author

@ecraig12345 Can you please review

@hetanthakkar
Copy link
Contributor Author

@ecraig12345 Can you please review this PR as well
#18197

ecraig12345
ecraig12345 approved these changes May 25, 2021
View reviewed changes
Copy link
Member

@ecraig12345 ecraig12345 left a comment

Choose a reason for hiding this comment

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

Approved with one minor suggestion. Thanks!

change/@fluentui-react-406968b0-014c-4861-8a06-0af485c00635.json Outdated Show resolved Hide resolved
@ecraig12345 ecraig12345 requested a review from a team as a code owner May 25, 2021 20:51
@ecraig12345 ecraig12345 changed the title Modified the list component to re-render when onRenderCell prop changes List: change type of version prop to any May 25, 2021
@ecraig12345 ecraig12345 merged commit 4301b99 into microsoft:master May 25, 2021
@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/react@v8.15.0 has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot msft-fluent-ui-bot mentioned this pull request May 26, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ecraig12345 ecraig12345 ecraig12345 approved these changes

@ThomasMichon ThomasMichon Awaiting requested review from ThomasMichon ThomasMichon is a code owner

Assignees
No one assigned
Labels
Component: List PR: API Modified
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fluent UI List - onRenderCell change does not cause re-render and update
4 participants
@hetanthakkar @fabricteam @ecraig12345 @msft-fluent-ui-bot