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

react-accordion - Simplify prop merging #18813

Merged
merged 5 commits into from
Jul 9, 2021
Merged

react-accordion - Simplify prop merging #18813

merged 5 commits into from
Jul 9, 2021

Conversation

bsunderhus
Copy link
Contributor

Pull request checklist

  • Include a change request file using $ yarn change

Description of changes

Implements changes proposed by the RFC #18721 on react-accordion and react-aria packages

@bsunderhus bsunderhus requested a review from ling1726 July 2, 2021 12:16
@bsunderhus bsunderhus self-assigned this Jul 2, 2021
@bsunderhus bsunderhus requested a review from a team July 2, 2021 12:16
@size-auditor
Copy link

size-auditor bot commented Jul 2, 2021 •
edited

Asset size changes

Project Bundle Baseline Size New Size Difference
office-ui-fabric-react fluentui-react-components-Accordion 14.859 kB 14.138 kB BelowBaseline     -721 bytes

ExceedsTolerance Over Tolerance (1024 B) ExceedsBaseline Over Baseline BelowBaseline Below Baseline New New Deleted  Removed 1 kB = 1000 B

Baseline commit: f99a295501d015d474f3c9c99a881dbcdfa3bd69 (build)

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jul 2, 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 d2275f4:

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

@fabricteam
Copy link
Collaborator

fabricteam commented Jul 2, 2021 •
edited

Perf Analysis (@fluentui/react)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 805 795 5000
BaseButton mount 874 877 5000
Breadcrumb mount 2569 2593 1000
ButtonNext mount 507 499 5000
Checkbox mount 1493 1497 5000
CheckboxBase mount 1262 1266 5000
ChoiceGroup mount 4655 4629 5000
ComboBox mount 969 974 1000
CommandBar mount 9970 9997 1000
ContextualMenu mount 6155 6180 1000
DefaultButton mount 1104 1128 5000
DetailsRow mount 3633 3670 5000
DetailsRowFast mount 3606 3673 5000
DetailsRowNoStyles mount 3511 3486 5000
Dialog mount 2127 2129 1000
DocumentCardTitle mount 152 142 1000
Dropdown mount 3182 3217 5000
FluentProviderNext mount 7234 7172 5000
FocusTrapZone mount 1780 1766 5000
FocusZone mount 1783 1774 5000
IconButton mount 1686 1712 5000
Label mount 334 343 5000
Layer mount 1778 1735 5000
Link mount 445 457 5000
MakeStyles mount 1767 1786 50000
MenuButton mount 1448 1437 5000
MessageBar mount 1993 2027 5000
Nav mount 3183 3175 1000
OverflowSet mount 1020 1038 5000
Panel mount 2051 2041 1000
Persona mount 826 833 1000
Pivot mount 1404 1384 1000
PrimaryButton mount 1263 1277 5000
Rating mount 7502 7402 5000
SearchBox mount 1288 1289 5000
Shimmer mount 2468 2484 5000
Slider mount 1886 1928 5000
SpinButton mount 4870 4850 5000
Spinner mount 416 424 5000
SplitButton mount 3300 3109 5000
Stack mount 496 484 5000
StackWithIntrinsicChildren mount 1437 1479 5000
StackWithTextChildren mount 4395 4384 5000
SwatchColorPicker mount 10083 9940 5000
Tabs mount 1369 1405 1000
TagPicker mount 2364 2404 5000
TeachingBubble mount 11714 11780 5000
Text mount 419 420 5000
TextField mount 1362 1323 5000
ThemeProvider mount 1162 1158 5000
ThemeProvider virtual-rerender 593 588 5000
Toggle mount 777 786 5000
buttonNative mount 112 111 5000

Perf Analysis (@fluentui/react-northstar)

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
AvatarMinimalPerf.default 194 173 1.12:1
ChatDuplicateMessagesPerf.default 291 267 1.09:1
TreeWith60ListItems.default 179 164 1.09:1
ListNestedPerf.default 554 526 1.05:1
PopupMinimalPerf.default 585 556 1.05:1
SegmentMinimalPerf.default 335 320 1.05:1
VideoMinimalPerf.default 621 595 1.04:1
AttachmentMinimalPerf.default 154 150 1.03:1
ButtonSlotsPerf.default 531 514 1.03:1
DividerMinimalPerf.default 346 337 1.03:1
FlexMinimalPerf.default 272 265 1.03:1
HeaderSlotsPerf.default 745 723 1.03:1
RadioGroupMinimalPerf.default 438 425 1.03:1
IconMinimalPerf.default 597 582 1.03:1
TextMinimalPerf.default 345 334 1.03:1
TextAreaMinimalPerf.default 497 482 1.03:1
AnimationMinimalPerf.default 403 397 1.02:1
ChatMinimalPerf.default 637 622 1.02:1
DialogMinimalPerf.default 740 723 1.02:1
GridMinimalPerf.default 333 325 1.02:1
ImageMinimalPerf.default 360 352 1.02:1
ItemLayoutMinimalPerf.default 1202 1182 1.02:1
PortalMinimalPerf.default 178 174 1.02:1
ProviderMinimalPerf.default 951 934 1.02:1
RefMinimalPerf.default 232 227 1.02:1
TooltipMinimalPerf.default 968 947 1.02:1
AttachmentSlotsPerf.default 1061 1049 1.01:1
ButtonMinimalPerf.default 163 162 1.01:1
EmbedMinimalPerf.default 4026 3973 1.01:1
HeaderMinimalPerf.default 351 349 1.01:1
InputMinimalPerf.default 1236 1226 1.01:1
ListCommonPerf.default 614 607 1.01:1
ListMinimalPerf.default 500 497 1.01:1
MenuMinimalPerf.default 817 812 1.01:1
ProviderMergeThemesPerf.default 1642 1628 1.01:1
SkeletonMinimalPerf.default 352 348 1.01:1
SplitButtonMinimalPerf.default 3634 3585 1.01:1
TableMinimalPerf.default 389 385 1.01:1
ToolbarMinimalPerf.default 927 922 1.01:1
TreeMinimalPerf.default 774 763 1.01:1
BoxMinimalPerf.default 344 345 1:1
ButtonOverridesMissPerf.default 1638 1641 1:1
CheckboxMinimalPerf.default 2676 2676 1:1
DatepickerMinimalPerf.default 5275 5269 1:1
DropdownMinimalPerf.default 3039 3042 1:1
ListWith60ListItems.default 622 619 1:1
MenuButtonMinimalPerf.default 1548 1547 1:1
TableManyItemsPerf.default 1841 1833 1:1
AccordionMinimalPerf.default 146 147 0.99:1
DropdownManyItemsPerf.default 641 646 0.99:1
LabelMinimalPerf.default 367 372 0.99:1
LayoutMinimalPerf.default 350 353 0.99:1
StatusMinimalPerf.default 654 663 0.99:1
CustomToolbarPrototype.default 3745 3773 0.99:1
CarouselMinimalPerf.default 442 453 0.98:1
LoaderMinimalPerf.default 664 680 0.98:1
RosterPerf.default 1131 1149 0.98:1
AlertMinimalPerf.default 261 269 0.97:1
CardMinimalPerf.default 530 544 0.97:1
ChatWithPopoverPerf.default 333 343 0.97:1
FormMinimalPerf.default 368 382 0.96:1
SliderMinimalPerf.default 1487 1551 0.96:1
ReactionMinimalPerf.default 356 375 0.95:1

@bsunderhus bsunderhus force-pushed the simplify-prop-merging-react-accordion branch 2 times, most recently from 0be81fc to 5f8a89e Compare July 2, 2021 17:26
@bsunderhus bsunderhus mentioned this pull request Jul 7, 2021
Closed
16 tasks
@bsunderhus bsunderhus force-pushed the simplify-prop-merging-react-accordion branch from 8f8d424 to 6d03aba Compare July 7, 2021 09:42
@bsunderhus bsunderhus enabled auto-merge (squash) July 7, 2021 10:50
layershifter
layershifter reviewed Jul 7, 2021
View reviewed changes
packages/react-accordion/src/components/AccordionPanel/useAccordionPanel.ts Show resolved Hide resolved
packages/react-accordion/src/components/Accordion/Accordion.types.ts

export type AccordionDefaultedProps = 'collapsible' | 'multiple' | 'navigable';
// export type AccordionDefaultedProps = 'collapsible' | 'multiple' | 'navigable';
Copy link
Member

Choose a reason for hiding this comment

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

Cruft?

Copy link
Member

Choose a reason for hiding this comment

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

This line was not removed as I see 😥

packages/react-accordion/src/components/Accordion/useAccordion.ts Show resolved Hide resolved
packages/react-accordion/src/components/Accordion/useAccordion.ts Show resolved Hide resolved
packages/react-accordion/src/components/AccordionHeader/useAccordionHeader.ts Show resolved Hide resolved
packages/react-accordion/src/components/AccordionHeader/useAccordionHeader.ts
}),
button: useARIAButton(props.button ?? button, {
id,
onClick: onAccordionHeaderClick,
Copy link
Member

Choose a reason for hiding this comment

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

This callback should be defined as an override to avoid collisions with user's input:

state.button.onClick = () => {}

@bsunderhus bsunderhus disabled auto-merge July 8, 2021 08:36
@bsunderhus bsunderhus force-pushed the simplify-prop-merging-react-accordion branch from 594eef8 to d2275f4 Compare July 9, 2021 06:46
@bsunderhus bsunderhus merged commit 5d52e38 into microsoft:master Jul 9, 2021
@bsunderhus bsunderhus deleted the simplify-prop-merging-react-accordion branch July 9, 2021 07:19
@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/react-accordion@v9.0.0-alpha.49 has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/react-aria@v9.0.0-alpha.9 has been released which incorporates this pull request.:tada:

Handy links:

PeterDraex pushed a commit to PeterDraex/fluentui that referenced this pull request Aug 6, 2021
@bsunderhus @PeterDraex
react-accordion - Simplify prop merging (microsoft#18813)
4166b81 
* Updates react-accordion & react-aria

* Updates API and interfaces

* Change files

* Updates shorthands types to slot

* Updates uninitializedState to initialState
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@layershifter layershifter layershifter left review comments

@ling1726 ling1726 ling1726 approved these changes

Assignees

@bsunderhus bsunderhus

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@bsunderhus @fabricteam @msft-fluent-ui-bot @layershifter @ling1726