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(Toolbar): fix positioning for overflow ToolbarMenu #12390

Merged
merged 11 commits into from
Mar 24, 2020

Conversation

layershifter
Copy link
Member

@layershifter layershifter commented Mar 23, 2020

Pull request checklist

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

Description of changes

This PR fixes positioning for ToolbarMenu in overflowItem inside Toolbar. Adds a screener tests to avoid regressions.

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 Mar 23, 2020
@layershifter layershifter changed the title fix(Toolbar): fix positioning for overflow ToolbarItem fix(Toolbar): fix positioning for overflow ToolbarMenu Mar 23, 2020
@msft-github-bot
Copy link
Contributor

msft-github-bot commented Mar 23, 2020

Perf Analysis

No significant results to display.

All results

Scenario Master Ticks PR Ticks Status
BaseButton 852 850
BaseButton (experiments) 1011 973
DefaultButton 1181 1199
DefaultButton (experiments) 1966 2014
DetailsRow 3567 3568
DetailsRow (fast icons) 3600 3569
DetailsRow without styles 3373 3245
DocumentCardTitle with truncation 1510 1530
MenuButton 1408 1549
MenuButton (experiments) 3741 3910
PrimaryButton 1313 1270
PrimaryButton (experiments) 2035 2024
SplitButton 3259 2996
SplitButton (experiments) 7109 7121
Stack 490 487
Stack with Intrinsic children 1151 1154
Stack with Text children 4433 4377
Text 383 436
Toggle 896 908
Toggle (experiments) 2264 2322
button 62 68

Perf Analysis (Fluent)

Perf comparison
Status Scenario Fluent TPI Fabric TPI Ratio Iterations Ticks
🔧 Avatar.Fluent 0.61 0.54 1.13:1 2000 1224
🦄 Button.Fluent 0.11 0.2 0.55:1 5000 540
🔧 Checkbox.Fluent 0.81 0.45 1.8:1 1000 811
🔧 Dialog.Fluent 0.43 0.22 1.95:1 5000 2145
🔧 Dropdown.Fluent 3.74 0.52 7.19:1 1000 3741
🔧 Icon.Fluent 0.21 0.05 4.2:1 5000 1042
🎯 Image.Fluent 0.08 0.11 0.73:1 5000 379
🔧 Slider.Fluent 1.71 0.46 3.72:1 1000 1708
🔧 Text.Fluent 0.09 0.02 4.5:1 5000 428
🦄 Tooltip.Fluent 0.14 17.82 0.01:1 5000 705

🔧 Needs work     🎯 On target     🦄 Amazing

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
TextMinimalPerf.default 487 386 1.26:1
PortalMinimalPerf.default 364 298 1.22:1
Avatar.Fluent 1224 1064 1.15:1
FlexMinimalPerf.default 314 279 1.13:1
ChatDuplicateMessagesPerf.default 510 456 1.12:1
Icon.Fluent 1042 957 1.09:1
AttachmentMinimalPerf.default 949 879 1.08:1
PopupMinimalPerf.default 257 239 1.08:1
TooltipMinimalPerf.default 985 914 1.08:1
GridMinimalPerf.default 1081 1012 1.07:1
ProviderMinimalPerf.default 689 646 1.07:1
Checkbox.Fluent 811 760 1.07:1
CarouselMinimalPerf.default 2106 1987 1.06:1
SegmentMinimalPerf.default 1247 1174 1.06:1
TreeWith60ListItems.default 244 230 1.06:1
AvatarMinimalPerf.default 583 555 1.05:1
ButtonMinimalPerf.default 173 164 1.05:1
ButtonSlotsPerf.default 655 625 1.05:1
ListMinimalPerf.default 511 487 1.05:1
RadioGroupMinimalPerf.default 657 626 1.05:1
RefMinimalPerf.default 201 191 1.05:1
ImageMinimalPerf.default 376 363 1.04:1
ItemLayoutMinimalPerf.default 2286 2197 1.04:1
LabelMinimalPerf.default 433 418 1.04:1
TextAreaMinimalPerf.default 3340 3199 1.04:1
CustomToolbarPrototype.default 3931 3785 1.04:1
Slider.Fluent 1708 1650 1.04:1
ChatWithPopoverPerf.default 598 583 1.03:1
DropdownMinimalPerf.default 3822 3703 1.03:1
Button.Fluent 540 522 1.03:1
BoxMinimalPerf.default 400 392 1.02:1
HeaderMinimalPerf.default 670 657 1.02:1
HierarchicalTreeMinimalPerf.default 1098 1073 1.02:1
LayoutMinimalPerf.default 704 690 1.02:1
ProviderMergeThemesPerf.default 1433 1408 1.02:1
SplitButtonMinimalPerf.default 12968 12717 1.02:1
AnimationMinimalPerf.default 685 677 1.01:1
DialogMinimalPerf.default 2062 2041 1.01:1
TreeMinimalPerf.default 1399 1390 1.01:1
AttachmentSlotsPerf.default 3673 3689 1:1
CardMinimalPerf.default 457 457 1:1
HeaderSlotsPerf.default 1875 1867 1:1
LoaderMinimalPerf.default 1120 1121 1:1
MenuButtonMinimalPerf.default 1759 1753 1:1
TableMinimalPerf.default 800 801 1:1
Text.Fluent 428 430 1:1
EmbedMinimalPerf.default 5440 5468 0.99:1
FormMinimalPerf.default 1090 1101 0.99:1
ListCommonPerf.default 1121 1138 0.99:1
ListNestedPerf.default 1061 1075 0.99:1
ListWith60ListItems.default 1236 1248 0.99:1
MenuMinimalPerf.default 2096 2113 0.99:1
VideoMinimalPerf.default 993 1004 0.99:1
Tooltip.Fluent 705 712 0.99:1
AccordionMinimalPerf.default 300 310 0.97:1
Dropdown.Fluent 3741 3844 0.97:1
Image.Fluent 379 390 0.97:1
DividerMinimalPerf.default 1076 1116 0.96:1
IconMinimalPerf.default 459 480 0.96:1
Dialog.Fluent 2145 2223 0.96:1
DropdownManyItemsPerf.default 1545 1627 0.95:1
InputMinimalPerf.default 1067 1119 0.95:1
ReactionMinimalPerf.default 2531 2697 0.94:1
SliderMinimalPerf.default 1540 1645 0.94:1
ChatMinimalPerf.default 630 681 0.93:1
StatusMinimalPerf.default 714 769 0.93:1
ToolbarMinimalPerf.default 1211 1307 0.93:1
CheckboxMinimalPerf.default 3305 3597 0.92:1
AlertMinimalPerf.default 616 735 0.84:1

Copy link
Member

@miroslavstastny miroslavstastny left a comment

Choose a reason for hiding this comment

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

The example does not work in RTL. Don't we want to add RTL example as well?


React.useLayoutEffect(() => {
if (node) {
node.contentDocument.documentElement.style.fontSize = '14px';
Copy link
Member

Choose a reason for hiding this comment

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

nit: I would also set a background to make the frame obvious.

@layershifter layershifter merged commit 3ad8d6c into master Mar 24, 2020
@layershifter layershifter deleted the fix/toolbar-pos-jumps branch March 24, 2020 14:06
miroslavstastny pushed a commit that referenced this pull request Apr 15, 2020
* fix(Toolbar): fix positioning for overflow ToolbarItem

* add border

* add RTL sample

* add changelog entry

* update entry

* update steps

* update steps

* update steps

* update steps

* add duplicate for RTL

* one more update

(cherry picked from commit 3ad8d6c)
DuanShaolong pushed a commit to DuanShaolong/fluentui that referenced this pull request Apr 27, 2020
* fix(Toolbar): fix positioning for overflow ToolbarItem

* add border

* add RTL sample

* add changelog entry

* update entry

* update steps

* update steps

* update steps

* update steps

* add duplicate for RTL

* one more update
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fluent UI react-northstar (v0) Work related to Fluent UI V0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants