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

chore(Accordion): Convert to RFC #12876

Merged
merged 6 commits into from
Apr 30, 2020
Merged

chore(Accordion): Convert to RFC #12876

merged 6 commits into from
Apr 30, 2020

Conversation

assuncaocharles
Copy link
Contributor

@assuncaocharles assuncaocharles commented Apr 25, 2020 •
edited by msft-github-bot

Pull request checklist

  • Addresses an existing issue: Fixes #0000
  • Include a change request file using $ yarn change

BREAKING CHANGES

This PR converts Accordion component to be functional. Not passing any prop to styles functions.

Related to #12237

Focus areas to test

(optional)

Microsoft Reviewers: Open in CodeFlow

@msft-github-bot msft-github-bot added the Fluent UI react-northstar (v0) Work related to Fluent UI V0 label Apr 25, 2020
@assuncaocharles assuncaocharles changed the title Chore/accordion rfc chore(Accordion): Convert to RFC Apr 25, 2020
@assuncaocharles
Copy link
Contributor Author

assuncaocharles commented Apr 25, 2020 •
edited

If I understood correctly, screener is complaining that before active elements didn't had outline and now they have ( has it suppose to be ). So I think it should be fine, @layershifter @pompomon ?

@size-auditor
Copy link

size-auditor bot commented Apr 25, 2020 •
edited

Asset size changes

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

Baseline commit: 30eb4f6d16830eae0f8080b6e6ad3b48fcd88810 (build)

@msft-github-bot
Copy link
Contributor

msft-github-bot commented Apr 25, 2020 •
edited

Perf Analysis

No significant results to display.

All results

Scenario Master Ticks PR Ticks Iterations Status
BaseButton 734 752 5000
Checkbox 1495 1461 5000
CheckboxBase 1178 1171 5000
ChoiceGroup 4545 4533 5000
ComboBox 887 844 1000
CommandBar 6988 7000 1000
ContextualMenu 13515 13525 1000
DefaultButton 955 952 5000
DetailsRow 3135 3105 5000
DetailsRow (fast icons) 3177 3169 5000
DetailsRow without styles 2965 2879 5000
Dialog 1360 1346 1000
DocumentCardTitle with truncation 1544 1537 1000
Dropdown 2231 2207 5000
FocusZone 1457 1466 5000
IconButton 1557 1569 5000
Label 254 284 5000
Link 409 408 5000
MenuButton 1296 1300 5000
Nav 2882 2830 1000
Panel 1348 1344 1000
Persona 754 761 1000
Pivot 1324 1229 1000
PrimaryButton 1112 1113 5000
SearchBox 1133 1189 5000
Slider 1378 1327 5000
Spinner 358 372 5000
SplitButton 2796 2835 5000
Stack 432 428 5000
Stack with Intrinsic children 1035 1032 5000
Stack with Text children 3774 3759 5000
TagPicker 2522 2552 5000
Text 338 331 5000
TextField 1274 1296 5000
Toggle 796 798 5000
button 65 61 5000

Perf Analysis (Fluent)

⚠️ 1 potential perf regressions detected

Potential regressions comparing to master

Scenario Current PR Ticks Baseline Ticks Ratio Regression Analysis
AccordionMinimalPerf.default 135 183 0.74:1 analysis
Perf comparison
Status Scenario Fluent TPI Fabric TPI Ratio Iterations Ticks
🎯 Avatar.Fluent 0.46 0.46 1:1 2000 916
🦄 Button.Fluent 0.1 0.18 0.56:1 5000 484
🔧 Checkbox.Fluent 0.64 0.34 1.88:1 1000 643
🔧 Dialog.Fluent 0.34 0.19 1.79:1 5000 1678
🔧 Dropdown.Fluent 3.22 0.4 8.05:1 1000 3222
🔧 Icon.Fluent 0.13 0.04 3.25:1 5000 636
🎯 Image.Fluent 0.07 0.1 0.7:1 5000 351
🔧 Slider.Fluent 1.35 0.32 4.22:1 1000 1349
🔧 Text.Fluent 0.06 0.02 3:1 5000 319
🦄 Tooltip.Fluent 0.08 17.68 0:1 5000 423

🔧 Needs work     🎯 On target     🦄 Amazing

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
AttachmentMinimalPerf.default 147 132 1.11:1
PortalMinimalPerf.default 296 276 1.07:1
TextAreaMinimalPerf.default 431 403 1.07:1
AvatarMinimalPerf.default 479 451 1.06:1
ButtonMinimalPerf.default 152 143 1.06:1
Avatar.Fluent 916 861 1.06:1
Image.Fluent 351 331 1.06:1
AnimationMinimalPerf.default 587 558 1.05:1
ChatWithPopoverPerf.default 572 544 1.05:1
LayoutMinimalPerf.default 518 495 1.05:1
InputMinimalPerf.default 980 943 1.04:1
ListMinimalPerf.default 442 427 1.04:1
Checkbox.Fluent 643 619 1.04:1
AttachmentSlotsPerf.default 1092 1064 1.03:1
ChatMinimalPerf.default 575 560 1.03:1
ListNestedPerf.default 860 839 1.03:1
StatusMinimalPerf.default 631 611 1.03:1
TableMinimalPerf.default 356 346 1.03:1
TreeWith60ListItems.default 213 207 1.03:1
Dialog.Fluent 1678 1630 1.03:1
CheckboxMinimalPerf.default 2794 2734 1.02:1
HeaderMinimalPerf.default 460 449 1.02:1
TreeMinimalPerf.default 1092 1072 1.02:1
VideoMinimalPerf.default 565 554 1.02:1
Dropdown.Fluent 3222 3152 1.02:1
Text.Fluent 319 313 1.02:1
DropdownManyItemsPerf.default 1287 1279 1.01:1
GridMinimalPerf.default 612 605 1.01:1
LoaderMinimalPerf.default 739 729 1.01:1
RadioGroupMinimalPerf.default 544 537 1.01:1
SliderMinimalPerf.default 1354 1344 1.01:1
TextMinimalPerf.default 302 300 1.01:1
CustomToolbarPrototype.default 3562 3511 1.01:1
TooltipMinimalPerf.default 687 681 1.01:1
Button.Fluent 484 480 1.01:1
Slider.Fluent 1349 1335 1.01:1
CardMinimalPerf.default 510 508 1:1
DividerMinimalPerf.default 666 668 1:1
FlexMinimalPerf.default 264 264 1:1
HierarchicalTreeMinimalPerf.default 947 948 1:1
ListCommonPerf.default 904 903 1:1
PopupMinimalPerf.default 239 240 1:1
ProviderMinimalPerf.default 634 632 1:1
SplitButtonMinimalPerf.default 3164 3172 1:1
BoxMinimalPerf.default 295 299 0.99:1
CarouselMinimalPerf.default 566 569 0.99:1
ChatDuplicateMessagesPerf.default 393 395 0.99:1
DialogMinimalPerf.default 1633 1649 0.99:1
FormMinimalPerf.default 670 675 0.99:1
HeaderSlotsPerf.default 1369 1377 0.99:1
ListWith60ListItems.default 1077 1089 0.99:1
MenuMinimalPerf.default 1719 1740 0.99:1
ProviderMergeThemesPerf.default 1567 1579 0.99:1
ReactionMinimalPerf.default 1739 1765 0.99:1
SegmentMinimalPerf.default 847 854 0.99:1
EmbedMinimalPerf.default 4091 4158 0.98:1
MenuButtonMinimalPerf.default 1420 1452 0.98:1
ToolbarMinimalPerf.default 909 924 0.98:1
DropdownMinimalPerf.default 3162 3270 0.97:1
ItemLayoutMinimalPerf.default 1511 1555 0.97:1
LabelMinimalPerf.default 355 367 0.97:1
RefMinimalPerf.default 188 194 0.97:1
IconMinimalPerf.default 610 632 0.97:1
Icon.Fluent 636 656 0.97:1
Tooltip.Fluent 423 434 0.97:1
AlertMinimalPerf.default 263 273 0.96:1
ImageMinimalPerf.default 320 334 0.96:1
ButtonSlotsPerf.default 568 598 0.95:1

kolaps33
kolaps33 reviewed Apr 28, 2020
View reviewed changes
Copy link
Contributor

@kolaps33 kolaps33 left a comment

Choose a reason for hiding this comment

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

I took look on the PR and did some basic tests with reader that there is no regression. And I didn't find anything.
From the code as well it looks ok for me. :)
Would be good if someone who wrote tests could review as well as there are changes.... :)

packages/fluentui/react-northstar/src/components/Accordion/Accordion.tsx Show resolved Hide resolved
silviuaavram
silviuaavram approved these changes Apr 29, 2020
View reviewed changes
Copy link
Contributor

@silviuaavram silviuaavram 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 to me.

@assuncaocharles assuncaocharles merged commit c13c65f into microsoft:master Apr 30, 2020
@assuncaocharles assuncaocharles deleted the chore/accordion-rfc branch May 4, 2020 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mnajdova mnajdova mnajdova approved these changes

@silviuaavram silviuaavram silviuaavram approved these changes

@dvdzkwsk dvdzkwsk Awaiting requested review from dvdzkwsk

@dzearing dzearing Awaiting requested review from dzearing

@jurokapsiar jurokapsiar Awaiting requested review from jurokapsiar

@layershifter layershifter Awaiting requested review from layershifter

@levithomason levithomason Awaiting requested review from levithomason

@miroslavstastny miroslavstastny Awaiting requested review from miroslavstastny

@pompomon pompomon Awaiting requested review from pompomon

@kolaps33 kolaps33 Awaiting requested review from kolaps33

Assignees

@joschect joschect

Labels
Fluent UI react-northstar (v0) Work related to Fluent UI V0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@assuncaocharles @msft-github-bot @mnajdova @silviuaavram @kolaps33 @joschect