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

Fix for blocking modal/dialog always having 'alertdialog' role #18298

Merged
merged 5 commits into from
Jun 7, 2021

Conversation

tringakrasniqi
Copy link
Contributor

@tringakrasniqi tringakrasniqi commented May 24, 2021

Pull request checklist

Description of changes

A new prop is added 'isAlert' which defines if a modal/dialog will be an 'alertdialog' if true or a 'dialog' if false. This prop will ignore the isModeless and isBlocking props for setting the role. In case the 'isAlert' props is missing(undefined) then the other two props will be checked. Also, added two tests with the three props, and updated snapshots.

The documentation will be done in a separate PR.

Focus areas to test

The Modal and Dialog component with the new prop 'isAlert' and variations of 'isModeless' and 'isBlocking'.

@codesandbox-ci
Copy link

codesandbox-ci bot commented May 24, 2021

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 5c6ae45:

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

@tringakrasniqi tringakrasniqi force-pushed the @tringakrasniqi/15499 branch from f734a58 to 8300c93 Compare May 25, 2021 13:39
@size-auditor
Copy link

size-auditor bot commented May 25, 2021

Asset size changes

Project Bundle Baseline Size New Size Difference
office-ui-fabric-react fluentui-react-Modal 84.999 kB 85.035 kB ExceedsBaseline     36 bytes
office-ui-fabric-react fluentui-react-Dialog 195.181 kB 195.212 kB ExceedsBaseline     31 bytes

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

Baseline commit: 203ccaf025da07d4064f8e4983298f0c1972c67d (build)

@fabricteam
Copy link
Collaborator

fabricteam commented May 25, 2021

Perf Analysis (@fluentui/react)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 798 830 5000
BaseButton mount 900 884 5000
Breadcrumb mount 2591 2556 1000
ButtonNext mount 485 495 5000
Checkbox mount 1427 1427 5000
CheckboxBase mount 1314 1266 5000
ChoiceGroup mount 4612 4754 5000
ComboBox mount 968 1008 1000
CommandBar mount 9872 9737 1000
ContextualMenu mount 5856 5944 1000
DefaultButton mount 1094 1091 5000
DetailsRow mount 3510 3558 5000
DetailsRowFast mount 3633 3607 5000
DetailsRowNoStyles mount 3346 3398 5000
Dialog mount 2191 2098 1000
DocumentCardTitle mount 142 137 1000
Dropdown mount 3125 3181 5000
FocusTrapZone mount 1768 1702 5000
FocusZone mount 1737 1688 5000
IconButton mount 1682 1692 5000
Label mount 344 334 5000
Layer mount 1761 1749 5000
Link mount 464 452 5000
MakeStyles mount 1794 1779 50000
MenuButton mount 1400 1438 5000
MessageBar mount 1941 1942 5000
Nav mount 3149 3185 1000
OverflowSet mount 1003 1018 5000
Panel mount 2009 1993 1000
Persona mount 798 782 1000
Pivot mount 1374 1365 1000
PrimaryButton mount 1245 1245 5000
Rating mount 7537 7611 5000
SearchBox mount 1272 1285 5000
Shimmer mount 2493 2517 5000
Slider mount 1926 1921 5000
SpinButton mount 4808 4929 5000
Spinner mount 406 396 5000
SplitButton mount 3132 3029 5000
Stack mount 468 482 5000
StackWithIntrinsicChildren mount 1492 1504 5000
StackWithTextChildren mount 4498 4419 5000
SwatchColorPicker mount 9908 9995 5000
Tabs mount 1380 1392 1000
TagPicker mount 2366 2368 5000
TeachingBubble mount 11441 11646 5000
Text mount 402 413 5000
TextField mount 1352 1369 5000
ThemeProvider mount 1171 1185 5000
ThemeProvider virtual-rerender 637 628 5000
ThemeProviderNext mount 6833 6930 5000
Toggle mount 795 767 5000
buttonNative mount 109 111 5000

Perf Analysis (@fluentui/react-northstar)

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
AccordionMinimalPerf.default 153 138 1.11:1
FlexMinimalPerf.default 295 268 1.1:1
ButtonMinimalPerf.default 167 156 1.07:1
TableMinimalPerf.default 414 387 1.07:1
AttachmentMinimalPerf.default 152 143 1.06:1
DialogMinimalPerf.default 771 727 1.06:1
FormMinimalPerf.default 412 388 1.06:1
PortalMinimalPerf.default 175 166 1.05:1
SegmentMinimalPerf.default 355 339 1.05:1
AvatarMinimalPerf.default 193 185 1.04:1
BoxMinimalPerf.default 336 323 1.04:1
CardMinimalPerf.default 549 530 1.04:1
ChatDuplicateMessagesPerf.default 289 279 1.04:1
ToolbarMinimalPerf.default 919 880 1.04:1
ChatMinimalPerf.default 595 578 1.03:1
DividerMinimalPerf.default 350 341 1.03:1
HeaderMinimalPerf.default 349 339 1.03:1
RadioGroupMinimalPerf.default 446 433 1.03:1
ReactionMinimalPerf.default 376 365 1.03:1
AttachmentSlotsPerf.default 1127 1101 1.02:1
DatepickerMinimalPerf.default 5424 5332 1.02:1
GridMinimalPerf.default 322 316 1.02:1
ProviderMergeThemesPerf.default 1640 1612 1.02:1
TableManyItemsPerf.default 1858 1828 1.02:1
TextMinimalPerf.default 328 323 1.02:1
ButtonSlotsPerf.default 536 530 1.01:1
CheckboxMinimalPerf.default 2712 2689 1.01:1
DropdownMinimalPerf.default 3029 2987 1.01:1
LayoutMinimalPerf.default 350 346 1.01:1
ListWith60ListItems.default 625 619 1.01:1
MenuMinimalPerf.default 817 810 1.01:1
ProviderMinimalPerf.default 976 964 1.01:1
SliderMinimalPerf.default 1564 1551 1.01:1
SplitButtonMinimalPerf.default 3712 3668 1.01:1
CustomToolbarPrototype.default 3807 3758 1.01:1
CarouselMinimalPerf.default 452 454 1:1
InputMinimalPerf.default 1238 1235 1:1
ItemLayoutMinimalPerf.default 1223 1227 1:1
ListMinimalPerf.default 488 490 1:1
ListNestedPerf.default 534 534 1:1
MenuButtonMinimalPerf.default 1570 1564 1:1
TooltipMinimalPerf.default 962 959 1:1
TreeMinimalPerf.default 775 773 1:1
VideoMinimalPerf.default 611 614 1:1
EmbedMinimalPerf.default 4041 4081 0.99:1
HeaderSlotsPerf.default 726 736 0.99:1
ImageMinimalPerf.default 358 362 0.99:1
LabelMinimalPerf.default 369 373 0.99:1
ListCommonPerf.default 602 610 0.99:1
LoaderMinimalPerf.default 659 665 0.99:1
AnimationMinimalPerf.default 393 399 0.98:1
DropdownManyItemsPerf.default 654 669 0.98:1
PopupMinimalPerf.default 543 556 0.98:1
TextAreaMinimalPerf.default 469 479 0.98:1
TreeWith60ListItems.default 157 161 0.98:1
AlertMinimalPerf.default 256 264 0.97:1
ButtonOverridesMissPerf.default 1641 1693 0.97:1
RefMinimalPerf.default 226 234 0.97:1
SkeletonMinimalPerf.default 344 353 0.97:1
StatusMinimalPerf.default 653 680 0.96:1
ChatWithPopoverPerf.default 337 362 0.93:1
RosterPerf.default 1089 1173 0.93:1
IconMinimalPerf.default 571 624 0.92:1

Copy link
Contributor

@smhigley smhigley left a comment

Choose a reason for hiding this comment

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

Got a couple changes, but overall nice work! Let me know if you have any questions on how to write a test to check for a specific attribute value vs. a general snapshot test.

packages/react/src/components/Modal/Modal.types.ts Outdated Show resolved Hide resolved
packages/react/src/components/Modal/Modal.base.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@smhigley smhigley left a comment

Choose a reason for hiding this comment

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

This looks fantastic, thanks!

@smhigley smhigley merged commit e816200 into microsoft:master Jun 7, 2021
@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

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.

Blocking Dialog/Modal always gets assigned role "alertdialog" when IsBlocking is true
5 participants