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(Dialog): Convert to RFC #13359

Merged
merged 12 commits into from
May 28, 2020
Merged

chore(Dialog): Convert to RFC #13359

merged 12 commits into from
May 28, 2020

Conversation

assuncaocharles
Copy link
Contributor

Pull request checklist

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

BREAKING CHANGES

This PR converts Dialog component to be functional. Restricting props set that will be passed to styles functions.

Related to #12237

Prop sets

Dialog
backdrop

Focus areas to test

(optional)

@msft-github-bot msft-github-bot added the Fluent UI react-northstar (v0) Work related to Fluent UI V0 label May 27, 2020
@msft-github-bot
Copy link
Contributor

msft-github-bot commented May 27, 2020 •
edited

Perf Analysis

No significant results to display.

All results

Scenario Master Ticks PR Ticks Iterations Status
BaseButton 826 792 5000
ButtonNext 432 441 5000
Checkbox 1543 1544 5000
CheckboxBase 1267 1241 5000
CheckboxNext 1547 1581 5000
ChoiceGroup 4797 4916 5000
ComboBox 892 966 1000
CommandBar 7566 7561 1000
ContextualMenu 13502 13136 1000
DefaultButton 1042 1059 5000
DetailsRow 3530 3420 5000
DetailsRow (fast icons) 3549 3488 5000
DetailsRow without styles 3236 3299 5000
Dialog 1508 1444 1000
DocumentCardTitle with truncation 1969 1984 1000
Dropdown 2426 2425 5000
FocusZone 1738 1698 5000
IconButton 1674 1714 5000
Label 326 328 5000
Link 460 465 5000
LinkNext 419 435 5000
MenuButton 1444 1419 5000
Nav 3316 3202 1000
Panel 1433 1482 1000
Persona 863 851 1000
Pivot 1378 1384 1000
PrimaryButton 1196 1197 5000
SearchBox 1179 1252 5000
Slider 1471 1500 5000
SliderNext 1810 1862 5000
Spinner 386 382 5000
SplitButton 3071 3033 5000
Stack 475 477 5000
Stack with Intrinsic children 1972 1881 5000
Stack with Text children 4886 4949 5000
TagPicker 2771 2763 5000
Text 373 372 5000
TextField 1415 1335 5000
Toggle 841 856 5000
ToggleNext 831 838 5000
button 86 65 5000

Perf Analysis (Fluent)

⚠️ 2 potential perf regressions detected

Potential regressions comparing to master

Scenario Current PR Ticks Baseline Ticks Ratio Regression Analysis
Dialog.Fluent 742 2844 0.26:1 analysis
DialogMinimalPerf.default 724 2907 0.25:1 analysis
Perf comparison
Status Scenario Fluent TPI Fabric TPI Ratio Iterations Ticks
🔧 Avatar.Fluent 0.72 0.49 1.47:1 2000 1443
🦄 Button.Fluent 0.11 0.19 0.58:1 5000 533
🔧 Checkbox.Fluent 1.14 0.35 3.26:1 1000 1140
🎯 Dialog.Fluent 0.15 0.21 0.71:1 5000 742
🔧 Dropdown.Fluent 6.38 0.45 14.18:1 1000 6383
🔧 Icon.Fluent 0.14 0.05 2.8:1 5000 691
🦄 Image.Fluent 0.07 0.11 0.64:1 5000 356
🔧 Slider.Fluent 3.01 0.35 8.6:1 1000 3006
🔧 Text.Fluent 0.07 0.02 3.5:1 5000 359
🦄 Tooltip.Fluent 0.1 18.22 0.01:1 5000 501

🔧 Needs work     🎯 On target     🦄 Amazing

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
AttachmentMinimalPerf.default 160 134 1.19:1
Text.Fluent 359 313 1.15:1
ButtonMinimalPerf.default 178 158 1.13:1
ChatWithPopoverPerf.default 698 653 1.07:1
TreeMinimalPerf.default 1339 1251 1.07:1
AlertMinimalPerf.default 347 326 1.06:1
MenuMinimalPerf.default 854 807 1.06:1
AnimationMinimalPerf.default 684 649 1.05:1
EmbedMinimalPerf.default 3476 3300 1.05:1
FormMinimalPerf.default 385 368 1.05:1
PopupMinimalPerf.default 265 252 1.05:1
TreeWith60ListItems.default 291 276 1.05:1
Image.Fluent 356 340 1.05:1
Tooltip.Fluent 501 478 1.05:1
ProviderMergeThemesPerf.default 1924 1842 1.04:1
Checkbox.Fluent 1140 1098 1.04:1
Dropdown.Fluent 6383 6139 1.04:1
Slider.Fluent 3006 2897 1.04:1
BoxMinimalPerf.default 320 312 1.03:1
ChatDuplicateMessagesPerf.default 554 536 1.03:1
HeaderSlotsPerf.default 793 768 1.03:1
ListCommonPerf.default 1141 1109 1.03:1
RadioGroupMinimalPerf.default 403 392 1.03:1
TextMinimalPerf.default 339 330 1.03:1
CheckboxMinimalPerf.default 5153 5043 1.02:1
ListMinimalPerf.default 457 448 1.02:1
CustomToolbarPrototype.default 4986 4892 1.02:1
TooltipMinimalPerf.default 733 716 1.02:1
Button.Fluent 533 521 1.02:1
CardMinimalPerf.default 555 547 1.01:1
ProviderMinimalPerf.default 824 815 1.01:1
IconMinimalPerf.default 629 623 1.01:1
ToolbarMinimalPerf.default 843 833 1.01:1
Avatar.Fluent 1443 1432 1.01:1
Icon.Fluent 691 681 1.01:1
CarouselMinimalPerf.default 520 522 1:1
ChatMinimalPerf.default 577 577 1:1
InputMinimalPerf.default 1625 1633 1:1
PortalMinimalPerf.default 118 118 1:1
SegmentMinimalPerf.default 332 333 1:1
SplitButtonMinimalPerf.default 3856 3846 1:1
StatusMinimalPerf.default 641 639 1:1
AccordionMinimalPerf.default 132 133 0.99:1
AvatarMinimalPerf.default 747 758 0.99:1
DividerMinimalPerf.default 338 341 0.99:1
ImageMinimalPerf.default 335 337 0.99:1
ItemLayoutMinimalPerf.default 2291 2311 0.99:1
LabelMinimalPerf.default 375 378 0.99:1
LayoutMinimalPerf.default 391 396 0.99:1
ListWith60ListItems.default 1584 1593 0.99:1
DropdownMinimalPerf.default 6407 6508 0.98:1
FlexMinimalPerf.default 291 297 0.98:1
HeaderMinimalPerf.default 346 353 0.98:1
MenuButtonMinimalPerf.default 1652 1680 0.98:1
TableMinimalPerf.default 356 365 0.98:1
AttachmentSlotsPerf.default 1199 1240 0.97:1
DropdownManyItemsPerf.default 2058 2120 0.97:1
HierarchicalTreeMinimalPerf.default 410 423 0.97:1
ListNestedPerf.default 1062 1097 0.97:1
ReactionMinimalPerf.default 376 387 0.97:1
RefMinimalPerf.default 192 197 0.97:1
SliderMinimalPerf.default 2845 2961 0.96:1
VideoMinimalPerf.default 592 614 0.96:1
ButtonSlotsPerf.default 720 757 0.95:1
LoaderMinimalPerf.default 1125 1191 0.94:1
TextAreaMinimalPerf.default 411 442 0.93:1
GridMinimalPerf.default 1249 1401 0.89:1

@size-auditor
Copy link

size-auditor bot commented May 27, 2020 •
edited

Asset size changes

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

Baseline commit: 2bc351b11ce81110a29b0a28f2a25fd8d9076863 (build)

assuncaocharles and others added 5 commits May 27, 2020 23:19
@assuncaocharles @layershifter
Update packages/fluentui/react-northstar/src/components/Dialog/Dialog…
879ae2a 
….tsx

Co-authored-by: Oleksandr Fediashov <alexander.mcgarret@gmail.com>
@assuncaocharles @layershifter
Update packages/fluentui/react-northstar/src/components/Dialog/Dialog…
6b59aa3 
….tsx

Co-authored-by: Oleksandr Fediashov <alexander.mcgarret@gmail.com>
@assuncaocharles @layershifter
Update packages/fluentui/react-northstar/src/components/Dialog/Dialog…
7a6f1d8 
….tsx

Co-authored-by: Oleksandr Fediashov <alexander.mcgarret@gmail.com>
@assuncaocharles
Merge branch 'master' into chore/dialog-rfc
ed15c49
@assuncaocharles
fix(Dialog): useEffect
5fb424f
@assuncaocharles assuncaocharles merged commit e5adec2 into microsoft:master May 28, 2020
@assuncaocharles assuncaocharles deleted the chore/dialog-rfc branch May 28, 2020 10:14
miroslavstastny pushed a commit to levithomason/fluentui that referenced this pull request Jun 8, 2020
@assuncaocharles @layershifter @miroslavstastny
chore(Dialog): Convert to RFC (microsoft#13359)
be3c36c 
* chore(Dialog): Convert to RFC

* fix(Dialog): Add changelog

* Update packages/fluentui/react-northstar/src/components/Dialog/Dialog.tsx

Co-authored-by: Oleksandr Fediashov <alexander.mcgarret@gmail.com>

* Update packages/fluentui/react-northstar/src/components/Dialog/Dialog.tsx

Co-authored-by: Oleksandr Fediashov <alexander.mcgarret@gmail.com>

* Update packages/fluentui/react-northstar/src/components/Dialog/Dialog.tsx

Co-authored-by: Oleksandr Fediashov <alexander.mcgarret@gmail.com>

* fix(Dialog): useEffect

* chore(Dialog): move functions

* chore(Dialog): remove flag

* chore(Dialog): remove flag

Co-authored-by: Oleksandr Fediashov <alexander.mcgarret@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@layershifter layershifter layershifter approved these changes

@dvdzkwsk dvdzkwsk Awaiting requested review from dvdzkwsk

@dzearing dzearing Awaiting requested review from dzearing

@jurokapsiar jurokapsiar Awaiting requested review from jurokapsiar

@levithomason levithomason Awaiting requested review from levithomason

@miroslavstastny miroslavstastny Awaiting requested review from miroslavstastny

@mnajdova mnajdova Awaiting requested review from mnajdova

@pompomon pompomon Awaiting requested review from pompomon

Assignees

@ecraig12345 ecraig12345

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
4 participants
@assuncaocharles @msft-github-bot @layershifter @ecraig12345