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

fix(Dialog): Fix body scroll when open dialog #12574

Merged
merged 7 commits into from
Apr 14, 2020
Merged

fix(Dialog): Fix body scroll when open dialog #12574

merged 7 commits into from
Apr 14, 2020

Conversation

assuncaocharles
Copy link
Contributor

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

Pull request checklist

Description of changes

Prevents scroll on the body element toggling the overflow css prop

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 7, 2020
layershifter
layershifter previously requested changes Apr 7, 2020
View reviewed changes
Copy link
Member

@layershifter layershifter left a comment •
edited

Choose a reason for hiding this comment

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

I don't think that this will be enough. When we discussed this issue with @jurokapsiar we tried https://www.npmjs.com/package/body-scroll-lock which obviously does more things

Another question: should be Dialog scrollable without backdrop?

image

@layershifter
Copy link
Member

There is also a prototype for nested dialogs packages/fluentui/docs/src/prototypes/NestedPopupsAndDialogs that should work with these changes.

@size-auditor
Copy link

size-auditor bot commented Apr 7, 2020 •
edited

Asset size changes

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

Baseline commit: 3f8cd747174532bac3cb81c8a608c2b43f5ced82 (build)

@msft-github-bot
Copy link
Contributor

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

Perf Analysis

No significant results to display.

All results

Scenario Master Ticks PR Ticks Iterations Status
BaseButton 858 896 5000
Checkbox 1882 2034 5000
CheckboxBase 1481 1491 5000
ChoiceGroup 5593 5519 5000
ComboBox 978 981 1000
CommandBar 7368 7385 1000
DefaultButton 1084 1085 5000
DetailsRow 3454 3450 5000
DetailsRow (fast icons) 3436 3554 5000
DetailsRow without styles 3193 3106 5000
Dialog 1501 1436 1000
DocumentCardTitle with truncation 1524 1543 1000
Dropdown 3041 3001 5000
FocusZone 1548 1624 5000
IconButton 1770 1728 5000
Label 486 487 5000
Link 459 444 5000
MenuButton 1424 1396 5000
Nav 3096 3109 1000
Panel 1430 1458 1000
Persona 844 835 1000
Pivot 1279 1250 1000
PrimaryButton 1169 1170 5000
SearchBox 1510 1522 5000
Slider 1794 1902 5000
Spinner 365 376 5000
SplitButton 3055 3006 5000
Stack 481 478 5000
Stack with Intrinsic children 1073 1100 5000
Stack with Text children 4159 4233 5000
TagPicker 2724 2712 5000
Text 373 379 5000
TextField 1729 1707 5000
Toggle 880 852 5000
button 63 62 5000

Perf Analysis (Fluent)

Perf comparison
Status Scenario Fluent TPI Fabric TPI Ratio Iterations Ticks
🔧 Avatar.Fluent 0.57 0.51 1.12:1 2000 1143
🦄 Button.Fluent 0.11 0.2 0.55:1 5000 542
🔧 Checkbox.Fluent 0.75 0.41 1.83:1 1000 747
🔧 Dialog.Fluent 0.42 0.21 2:1 5000 2102
🔧 Dropdown.Fluent 3.84 0.48 8:1 1000 3842
🔧 Icon.Fluent 0.15 0.05 3:1 5000 762
🎯 Image.Fluent 0.09 0.11 0.82:1 5000 441
🔧 Slider.Fluent 1.62 0.44 3.68:1 1000 1624
🔧 Text.Fluent 0.08 0.02 4:1 5000 395
🦄 Tooltip.Fluent 0.13 17.54 0.01:1 5000 652

🔧 Needs work     🎯 On target     🦄 Amazing

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
ButtonMinimalPerf.default 187 157 1.19:1
PopupMinimalPerf.default 277 234 1.18:1
RefMinimalPerf.default 217 186 1.17:1
HeaderMinimalPerf.default 644 585 1.1:1
BoxMinimalPerf.default 386 353 1.09:1
DialogMinimalPerf.default 2390 2183 1.09:1
AttachmentMinimalPerf.default 181 167 1.08:1
ImageMinimalPerf.default 409 383 1.07:1
SliderMinimalPerf.default 1702 1604 1.06:1
IconMinimalPerf.default 718 677 1.06:1
AnimationMinimalPerf.default 807 768 1.05:1
AvatarMinimalPerf.default 647 619 1.05:1
LabelMinimalPerf.default 457 435 1.05:1
LoaderMinimalPerf.default 1160 1106 1.05:1
Button.Fluent 542 516 1.05:1
Icon.Fluent 762 728 1.05:1
FlexMinimalPerf.default 344 330 1.04:1
LayoutMinimalPerf.default 750 719 1.04:1
ToolbarMinimalPerf.default 1260 1217 1.04:1
TreeWith60ListItems.default 256 247 1.04:1
CheckboxMinimalPerf.default 3710 3603 1.03:1
EmbedMinimalPerf.default 5934 5756 1.03:1
FormMinimalPerf.default 1073 1045 1.03:1
HeaderSlotsPerf.default 1887 1838 1.03:1
HierarchicalTreeMinimalPerf.default 1152 1118 1.03:1
ListNestedPerf.default 1106 1079 1.03:1
ProviderMinimalPerf.default 687 667 1.03:1
Dialog.Fluent 2102 2035 1.03:1
Text.Fluent 395 383 1.03:1
AlertMinimalPerf.default 689 673 1.02:1
ChatDuplicateMessagesPerf.default 473 464 1.02:1
GridMinimalPerf.default 969 954 1.02:1
PortalMinimalPerf.default 336 331 1.02:1
RadioGroupMinimalPerf.default 616 606 1.02:1
ReactionMinimalPerf.default 2699 2645 1.02:1
SegmentMinimalPerf.default 1228 1202 1.02:1
CustomToolbarPrototype.default 3905 3833 1.02:1
TooltipMinimalPerf.default 957 934 1.02:1
Image.Fluent 441 434 1.02:1
Tooltip.Fluent 652 637 1.02:1
ButtonSlotsPerf.default 682 672 1.01:1
DividerMinimalPerf.default 1154 1140 1.01:1
MenuButtonMinimalPerf.default 1804 1793 1.01:1
ProviderMergeThemesPerf.default 1544 1522 1.01:1
TextAreaMinimalPerf.default 3301 3265 1.01:1
TreeMinimalPerf.default 1314 1305 1.01:1
Checkbox.Fluent 747 740 1.01:1
Slider.Fluent 1624 1603 1.01:1
AttachmentSlotsPerf.default 2850 2837 1:1
CardMinimalPerf.default 482 481 1:1
DropdownMinimalPerf.default 4138 4156 1:1
ItemLayoutMinimalPerf.default 2235 2229 1:1
SplitButtonMinimalPerf.default 3941 3925 1:1
AccordionMinimalPerf.default 278 281 0.99:1
InputMinimalPerf.default 1099 1107 0.99:1
ListCommonPerf.default 1153 1165 0.99:1
ListMinimalPerf.default 516 519 0.99:1
MenuMinimalPerf.default 2160 2182 0.99:1
TableMinimalPerf.default 752 760 0.99:1
Avatar.Fluent 1143 1159 0.99:1
ListWith60ListItems.default 1368 1389 0.98:1
TextMinimalPerf.default 371 379 0.98:1
Dropdown.Fluent 3842 3910 0.98:1
CarouselMinimalPerf.default 684 703 0.97:1
ChatMinimalPerf.default 675 695 0.97:1
ChatWithPopoverPerf.default 672 700 0.96:1
DropdownManyItemsPerf.default 1601 1671 0.96:1
VideoMinimalPerf.default 977 1022 0.96:1
StatusMinimalPerf.default 749 787 0.95:1

@assuncaocharles assuncaocharles force-pushed the fix/dialog branch 2 times, most recently from 054b169 to 9dc6eb5 Compare April 7, 2020 12:37
@assuncaocharles
Copy link
Contributor Author

assuncaocharles commented Apr 7, 2020 •
edited

I don't think that this will be enough. When we discussed this issue with @jurokapsiar we tried https://www.npmjs.com/package/body-scroll-lock which obviously does more things

Another question: should be Dialog scrollable without backdrop?

image

Updated using the suggested package

@assuncaocharles assuncaocharles force-pushed the fix/dialog branch 3 times, most recently from 61187c5 to 9619eb5 Compare April 9, 2020 19:46
@assuncaocharles assuncaocharles force-pushed the fix/dialog branch 2 times, most recently from 3275c25 to 079b468 Compare April 10, 2020 22:40
layershifter
layershifter reviewed Apr 14, 2020
View reviewed changes
yarn.lock Outdated Show resolved Hide resolved
@assuncaocharles assuncaocharles merged commit afdc85c into microsoft:master Apr 14, 2020
@assuncaocharles assuncaocharles deleted the fix/dialog branch April 23, 2020 14:52
DuanShaolong pushed a commit to DuanShaolong/fluentui that referenced this pull request Apr 27, 2020
@assuncaocharles @chpalac
@layershifter @DuanShaolong
fix(Dialog): Fix body scroll when open dialog (microsoft#12574)
e1080c3 
* fix(Dialog): Fix body scroll when open dialog

* Fix(Dialog) Review changes

* Fix(Dialog) add change log

* fix(Dialog): Update yarn.lock

Co-Authored-By: Oleksandr Fediashov <alexander.mcgarret@gmail.com>

* fix(Dialog): Update changelog

* fix(Dialog): update changelog

Co-authored-by: Charles Assuncao <chassunc@microsoft.com>
Co-authored-by: Oleksandr Fediashov <alexander.mcgarret@gmail.com>
@assuncaocharles assuncaocharles mentioned this pull request Feb 18, 2021
Merged
2 tasks
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

@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.

<Dialog /> overlay doesn't disable the scroll at the top level when open.
7 participants
@assuncaocharles @layershifter @msft-github-bot @pompomon @mnajdova @joschect @chpalac