Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

chore(Tooltip): move to function component #2357

Merged
merged 10 commits into from
Feb 17, 2020
Merged

chore(Tooltip): move to function component #2357

merged 10 commits into from
Feb 17, 2020

Conversation

layershifter
Copy link
Member

@layershifter layershifter commented Feb 14, 2020 •
edited
Loading

BREAKING CHANGES

As in other components converted to hooks TooltipContent now passes only these props to styles: placement, pointing, open.

Performance

I used an example with 100 opened Tooltips to measure rendering speed. And it almost 2 times faster because of enabled caching (#2309).

yarn perf --filter *TooltipMin*
const TooltipMinimalPerf = () => (
  <>
    {_.times(100, i => (
      <Tooltip key={i} content="Sample">
        <div />
      </Tooltip>
    ))}
  </>
);

Before

Example min avg median max
TooltipMinimal.perf.tsx 74.38 78.47 78.12 104.65
TooltipMinimal.perf.tsx 74.92 78.98 78.19 98.78
TooltipMinimal.perf.tsx 74.99 78.18 77.71 91.64

After without caching

It's also a bit faster because Tooltip now completely omits styles computations.

Example min avg median max
TooltipMinimal.perf.tsx 69.55 76.33 74.1 92.98

After

Example min avg median max
TooltipMinimal.perf.tsx 33.29 38.66 36.8 49.7
TooltipMinimal.perf.tsx 34.18 37.9 36.77 49.52
TooltipMinimal.perf.tsx 33.87 39.07 36.63 51.7

@DustyTheBot
Copy link
Collaborator

DustyTheBot commented Feb 14, 2020 •
edited
Loading

Warnings
⚠️ 2 perf regressions detected

Perf comparison

Status Scenario Fluent TPI Fabric TPI Ratio Iterations Ticks
🔧 Avatar.Fluent 0.46 0.36 1.28:1 2000 920
🎯 Button.Fluent 0.12 0.16 0.75:1 1000 117
🔧 Checkbox.Fluent 0.74 0.28 2.64:1 1000 740
🔧 Dialog.Fluent 0.32 0.16 2:1 5000 1605
🔧 Dropdown.Fluent 3.19 0.34 9.38:1 1000 3186
🔧 Icon.Fluent 0.12 0.03 4:1 5000 591
🦄 Image.Fluent 0.04 0.07 0.57:1 5000 220
🔧 Slider.Fluent 1.4 0.29 4.83:1 1000 1402
🔧 Text.Fluent 0.05 0.02 2.5:1 5000 241
🦄 Tooltip.Fluent 0.09 14.11 0.01:1 5000 430

🔧 Needs work     🎯 On target     🦄 Amazing

Potential regressions comparing to master

Scenario Current PR Ticks Baseline Ticks Ratio
StatusMinimalPerf.default 231 228 1.01:1
Tooltip.Fluent 430 591 0.73:1
Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
ChatDuplicateMessagesPerf.default 663 551 1.2:1
PortalMinimalPerf.default 247 209 1.18:1
AttachmentMinimalPerf.default 1118 958 1.17:1
Button.Fluent 117 103 1.14:1
ListNestedPerf.default 739 660 1.12:1
Icon.Fluent 591 535 1.1:1
FlexMinimalPerf.default 355 325 1.09:1
HeaderSlotsPerf.default 1451 1327 1.09:1
RefMinimalPerf.default 156 145 1.08:1
FormMinimalPerf.default 743 694 1.07:1
ImageMinimalPerf.default 230 216 1.06:1
SplitButtonMinimalPerf.default 11883 11242 1.06:1
DropdownMinimalPerf.default 3508 3333 1.05:1
LayoutMinimalPerf.default 532 511 1.04:1
TreeMinimalPerf.default 831 800 1.04:1
Tree60WithListItems.default 218 209 1.04:1
ChatWithPopoverPerf.default 585 567 1.03:1
PopupMinimalPerf.default 314 305 1.03:1
SliderMinimalPerf.default 1507 1465 1.03:1
TableMinimalPerf.default 555 537 1.03:1
AvatarMinimalPerf.default 519 507 1.02:1
ButtonSlotsPerf.default 578 565 1.02:1
EmbedMinimalPerf.default 6117 6009 1.02:1
ListWith60ListItems.default 146 143 1.02:1
MenuMinimalPerf.default 1843 1813 1.02:1
TextMinimalPerf.default 281 276 1.02:1
ToolbarMinimalPerf.default 684 669 1.02:1
VideoMinimalPerf.default 638 626 1.02:1
Dialog.Fluent 1605 1574 1.02:1
CarouselMinimalPerf.default 1891 1866 1.01:1
DividerMinimalPerf.default 847 840 1.01:1
HierarchicalTreeMinimalPerf.default 781 777 1.01:1
IconMinimalPerf.default 280 277 1.01:1
InputMinimalPerf.default 884 873 1.01:1
ListCommonPerf.default 770 766 1.01:1
MenuButtonMinimalPerf.default 1401 1382 1.01:1
Avatar.Fluent 920 908 1.01:1
Slider.Fluent 1402 1386 1.01:1
BoxMinimalPerf.default 222 223 1:1
Dropdown.Fluent 3186 3184 1:1
Text.Fluent 241 242 1:1
AttachmentSlotsPerf.default 3438 3462 0.99:1
ChatMinimalPerf.default 1625 1635 0.99:1
CheckboxMinimalPerf.default 3621 3648 0.99:1
TextAreaMinimalPerf.default 2767 2781 0.99:1
LoaderMinimalPerf.default 2262 2316 0.98:1
Checkbox.Fluent 740 752 0.98:1
ButtonMinimalPerf.default 116 119 0.97:1
HeaderMinimalPerf.default 411 425 0.97:1
ItemLayoutMinimalPerf.default 1658 1704 0.97:1
ProviderMinimalPerf.default 548 565 0.97:1
ReactionMinimalPerf.default 2375 2448 0.97:1
DialogMinimalPerf.default 1605 1671 0.96:1
LabelMinimalPerf.default 774 803 0.96:1
ProviderMergeThemesPerf.default 1020 1060 0.96:1
Image.Fluent 220 228 0.96:1
AccordionMinimalPerf.default 195 205 0.95:1
GridMinimalPerf.default 783 822 0.95:1
ListMinimalPerf.default 297 317 0.94:1
RadioGroupMinimalPerf.default 382 405 0.94:1
SegmentMinimalPerf.default 1198 1274 0.94:1
CustomToolbarPrototype.default 3185 3442 0.93:1
AnimationMinimalPerf.default 473 530 0.89:1
DropdownManyItemsPerf.default 401 452 0.89:1
AlertMinimalPerf.default 565 664 0.85:1
TooltipMinimalPerf.default 620 807 0.77:1

Generated by 🚫 dangerJS

@layershifter layershifter added 🚀 ready for review 🧰 fix Introduces fix for broken behavior. labels Feb 14, 2020
@DustyTheBot DustyTheBot mentioned this pull request Feb 14, 2020
Merged
mnajdova
mnajdova reviewed Feb 17, 2020
View reviewed changes
packages/react/src/components/Tooltip/Tooltip.tsx Outdated
@@ -68,6 +67,12 @@ export interface TooltipProps
/** Defines whether tooltip is displayed. */
open?: boolean

// TODO
Copy link
Contributor

Choose a reason for hiding this comment

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

Ups

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

mnajdova
mnajdova reviewed Feb 17, 2020
View reviewed changes
packages/react/src/components/Tooltip/Tooltip.tsx Outdated
e: React.MouseEvent | React.FocusEvent | React.KeyboardEvent,
data: TooltipProps,
) => void

/**
* TODO: should this be centralized?
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes please

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know what this means as it was added previously 🤔 I removed it for now

@layershifter layershifter merged commit fb28fc5 into master Feb 17, 2020
@layershifter layershifter deleted the chore/tooltip-to-hooks branch February 17, 2020 16:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mnajdova mnajdova mnajdova approved these changes

@dvdzkwsk dvdzkwsk Awaiting requested review from dvdzkwsk

@dzearing dzearing Awaiting requested review from dzearing dzearing is a code owner

@ecraig12345 ecraig12345 Awaiting requested review from ecraig12345 ecraig12345 is a code owner

@JasonGore JasonGore Awaiting requested review from JasonGore JasonGore is a code owner

@jdhuntington jdhuntington Awaiting requested review from jdhuntington

@kenotron kenotron Awaiting requested review from kenotron kenotron is a code owner

@levithomason levithomason Awaiting requested review from levithomason levithomason is a code owner

@miroslavstastny miroslavstastny Awaiting requested review from miroslavstastny miroslavstastny is a code owner

Assignees
No one assigned
Labels
🧰 fix Introduces fix for broken behavior. 🚀 ready for review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@layershifter @DustyTheBot @mnajdova