Update keytip functionality #20396
Merged
Update keytip functionality #20396
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This reverts commit 68fffc2.
📊 Bundle size report🤖 This report was generated against 0e0cf06c34f78d54db4468a602a17b0a5a30ac87 |
|
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 ee9d3c2:
|
Asset size changes
Baseline commit: 0e0cf06c34f78d54db4468a602a17b0a5a30ac87 (build) |
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| FluentProviderWithTheme | mount | 207 | 218 | 10 | Possible regression |
| FluentProviderWithTheme | virtual-rerender | 115 | 114 | 10 | Possible regression |
| buttonNative | mount | 152 | 135 | 5000 | Possible regression |
All results
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| Avatar | mount | 1120 | 1067 | 5000 | |
| BaseButton | mount | 1055 | 1067 | 5000 | |
| Breadcrumb | mount | 2755 | 2790 | 1000 | |
| ButtonNext | mount | 541 | 569 | 5000 | |
| Checkbox | mount | 1753 | 1830 | 5000 | |
| CheckboxBase | mount | 1441 | 1499 | 5000 | |
| ChoiceGroup | mount | 5273 | 5348 | 5000 | |
| ComboBox | mount | 1142 | 1065 | 1000 | |
| CommandBar | mount | 11033 | 10937 | 1000 | |
| ContextualMenu | mount | 7023 | 7004 | 1000 | |
| DefaultButton | mount | 1280 | 1347 | 5000 | |
| DetailsRow | mount | 4173 | 4173 | 5000 | |
| DetailsRowFast | mount | 4036 | 4126 | 5000 | |
| DetailsRowNoStyles | mount | 3970 | 3979 | 5000 | |
| Dialog | mount | 2688 | 2751 | 1000 | |
| DocumentCardTitle | mount | 197 | 181 | 1000 | |
| Dropdown | mount | 3559 | 3464 | 5000 | |
| FluentProviderNext | mount | 3374 | 3306 | 5000 | |
| FluentProviderWithTheme | mount | 207 | 218 | 10 | Possible regression |
| FluentProviderWithTheme | virtual-rerender | 115 | 114 | 10 | Possible regression |
| FluentProviderWithTheme | virtual-rerender-with-unmount | 241 | 248 | 10 | |
| FocusTrapZone | mount | 1892 | 1939 | 5000 | |
| FocusZone | mount | 1886 | 1894 | 5000 | |
| IconButton | mount | 1966 | 1985 | 5000 | |
| Label | mount | 386 | 373 | 5000 | |
| Layer | mount | 3261 | 3290 | 5000 | |
| Link | mount | 535 | 536 | 5000 | |
| MakeStyles | mount | 1960 | 1936 | 50000 | |
| MenuButton | mount | 1707 | 1632 | 5000 | |
| MessageBar | mount | 2138 | 2218 | 5000 | |
| Nav | mount | 3655 | 3607 | 1000 | |
| OverflowSet | mount | 1201 | 1228 | 5000 | |
| Panel | mount | 2617 | 2631 | 1000 | |
| Persona | mount | 927 | 930 | 1000 | |
| Pivot | mount | 1576 | 1664 | 1000 | |
| PrimaryButton | mount | 1459 | 1408 | 5000 | |
| Rating | mount | 8673 | 8712 | 5000 | |
| SearchBox | mount | 1502 | 1574 | 5000 | |
| Shimmer | mount | 2837 | 2906 | 5000 | |
| Slider | mount | 2187 | 2137 | 5000 | |
| SpinButton | mount | 5502 | 5377 | 5000 | |
| Spinner | mount | 481 | 473 | 5000 | |
| SplitButton | mount | 3473 | 3540 | 5000 | |
| Stack | mount | 580 | 590 | 5000 | |
| StackWithIntrinsicChildren | mount | 1925 | 1937 | 5000 | |
| StackWithTextChildren | mount | 5189 | 5383 | 5000 | |
| SwatchColorPicker | mount | 11312 | 11339 | 5000 | |
| Tabs | mount | 1565 | 1563 | 1000 | |
| TagPicker | mount | 2909 | 2871 | 5000 | |
| TeachingBubble | mount | 13701 | 13643 | 5000 | |
| Text | mount | 470 | 474 | 5000 | |
| TextField | mount | 1599 | 1566 | 5000 | |
| ThemeProvider | mount | 1251 | 1246 | 5000 | |
| ThemeProvider | virtual-rerender | 645 | 654 | 5000 | |
| ThemeProvider | virtual-rerender-with-unmount | 2082 | 2104 | 5000 | |
| Toggle | mount | 892 | 904 | 5000 | |
| buttonNative | mount | 152 | 135 | 5000 | Possible regression |
Perf Analysis (@fluentui/react-northstar)
Perf tests with no regressions
| Scenario | Current PR Ticks | Baseline Ticks | Ratio |
|---|---|---|---|
| ButtonMinimalPerf.default | 220 | 196 | 1.12:1 |
| AttachmentMinimalPerf.default | 204 | 183 | 1.11:1 |
| PortalMinimalPerf.default | 197 | 183 | 1.08:1 |
| ImageMinimalPerf.default | 439 | 411 | 1.07:1 |
| MenuMinimalPerf.default | 959 | 899 | 1.07:1 |
| IconMinimalPerf.default | 727 | 688 | 1.06:1 |
| TreeMinimalPerf.default | 920 | 872 | 1.06:1 |
| LayoutMinimalPerf.default | 422 | 402 | 1.05:1 |
| ListNestedPerf.default | 651 | 620 | 1.05:1 |
| ReactionMinimalPerf.default | 434 | 414 | 1.05:1 |
| RefMinimalPerf.default | 263 | 250 | 1.05:1 |
| AccordionMinimalPerf.default | 169 | 162 | 1.04:1 |
| BoxMinimalPerf.default | 386 | 371 | 1.04:1 |
| ButtonSlotsPerf.default | 634 | 611 | 1.04:1 |
| CardMinimalPerf.default | 656 | 630 | 1.04:1 |
| CarouselMinimalPerf.default | 536 | 516 | 1.04:1 |
| ChatMinimalPerf.default | 754 | 725 | 1.04:1 |
| ChatWithPopoverPerf.default | 438 | 423 | 1.04:1 |
| StatusMinimalPerf.default | 761 | 731 | 1.04:1 |
| AnimationMinimalPerf.default | 463 | 448 | 1.03:1 |
| CheckboxMinimalPerf.default | 2995 | 2903 | 1.03:1 |
| DialogMinimalPerf.default | 847 | 823 | 1.03:1 |
| DropdownManyItemsPerf.default | 786 | 760 | 1.03:1 |
| GridMinimalPerf.default | 393 | 383 | 1.03:1 |
| LabelMinimalPerf.default | 442 | 430 | 1.03:1 |
| RadioGroupMinimalPerf.default | 519 | 506 | 1.03:1 |
| SegmentMinimalPerf.default | 390 | 378 | 1.03:1 |
| TreeWith60ListItems.default | 217 | 210 | 1.03:1 |
| VideoMinimalPerf.default | 719 | 701 | 1.03:1 |
| DatepickerMinimalPerf.default | 6275 | 6128 | 1.02:1 |
| ListCommonPerf.default | 721 | 705 | 1.02:1 |
| LoaderMinimalPerf.default | 760 | 745 | 1.02:1 |
| PopupMinimalPerf.default | 647 | 636 | 1.02:1 |
| TextMinimalPerf.default | 379 | 371 | 1.02:1 |
| AlertMinimalPerf.default | 308 | 304 | 1.01:1 |
| EmbedMinimalPerf.default | 4621 | 4572 | 1.01:1 |
| FlexMinimalPerf.default | 314 | 311 | 1.01:1 |
| HeaderMinimalPerf.default | 417 | 411 | 1.01:1 |
| HeaderSlotsPerf.default | 867 | 855 | 1.01:1 |
| ListMinimalPerf.default | 559 | 553 | 1.01:1 |
| ListWith60ListItems.default | 720 | 713 | 1.01:1 |
| ButtonOverridesMissPerf.default | 1957 | 1958 | 1:1 |
| FormMinimalPerf.default | 484 | 483 | 1:1 |
| InputMinimalPerf.default | 1424 | 1418 | 1:1 |
| SkeletonMinimalPerf.default | 403 | 403 | 1:1 |
| SliderMinimalPerf.default | 1838 | 1830 | 1:1 |
| SplitButtonMinimalPerf.default | 4694 | 4684 | 1:1 |
| TableMinimalPerf.default | 455 | 453 | 1:1 |
| TooltipMinimalPerf.default | 1164 | 1164 | 1:1 |
| AvatarMinimalPerf.default | 222 | 225 | 0.99:1 |
| DropdownMinimalPerf.default | 3322 | 3346 | 0.99:1 |
| MenuButtonMinimalPerf.default | 1823 | 1835 | 0.99:1 |
| RosterPerf.default | 1334 | 1342 | 0.99:1 |
| ProviderMergeThemesPerf.default | 1796 | 1821 | 0.99:1 |
| TextAreaMinimalPerf.default | 587 | 592 | 0.99:1 |
| ToolbarMinimalPerf.default | 1052 | 1067 | 0.99:1 |
| AttachmentSlotsPerf.default | 1159 | 1187 | 0.98:1 |
| ChatDuplicateMessagesPerf.default | 336 | 343 | 0.98:1 |
| ProviderMinimalPerf.default | 1201 | 1239 | 0.97:1 |
| TableManyItemsPerf.default | 2101 | 2171 | 0.97:1 |
| ItemLayoutMinimalPerf.default | 1324 | 1375 | 0.96:1 |
| CustomToolbarPrototype.default | 4427 | 4643 | 0.95:1 |
| DividerMinimalPerf.default | 388 | 412 | 0.94:1 |
…my story update, adding some text to the keytipElement, and making the size of the area larger
kelseyyoung
approved these changes
Oct 28, 2021
…ips to not show up
jspurlin
changed the title
|
@khmakoto Can you review this? It's a keytip change that office online needs and the creator of keytips (Kelsey) also already approved the PR |
khmakoto
approved these changes
Nov 2, 2021
Comment on lines
319
to
+320
| }); | ||
| it('Process a node with no matching visible element and is a submenu in an overflow', () => { |
There was a problem hiding this comment.
nit:
Suggested change
| }); | |
| it('Process a node with no matching visible element and is a submenu in an overflow', () => { | |
| }); | |
| it('Process a node with no matching visible element and is a submenu in an overflow', () => { |
| } | ||
|
|
||
| // Since the matching nodes all have the same key sequence, | ||
| // Grab the first one build the correct selector |
There was a problem hiding this comment.
nit:
Suggested change
| // Grab the first one build the correct selector | |
| // Grab the first one to build the correct selector |
mlp73
pushed a commit
to mlp73/fluentui
that referenced
this pull request
Jan 17, 2022
* Make callouts aware of the WindowSegments * Revert "Make callouts aware of the WindowSegments" This reverts commit 68fffc2. * Keytips: Make keytips aware of the visibility of their targets * Change files * Fix the lint errors * Update keytip story * Actually update the story to use a DefaultButton instead of an html button * actually I just noticed there was already a ktp target span, undoing my story update, adding some text to the keytipElement, and making the size of the area larger * Update story because js was executing before first paint causing keytips to not show up
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull request checklist
$ yarn changeDescription of changes
This PR adds the ability for keytips to be more aware of if their targets are visible and 1) not show a keytip if the target is not visible 2) execute the correct onExecute (that corresponds to the visible target) if there are multiple of the same keytip registered.
Note, this can happen if a consumer is showing/hiding elements via css (e.g. responsive scaling) instead of unmounting a target that has a keytip.
Focus areas to test
Verified keytips behave as they did before, except when there are hidden elements, in which case they are now aware of this state. Verified normal controls and controls with in overflows.
I also added a unit test to cover this case