ReAdd: Keytips: Align keytipData with visible instance for dupes #28992
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
…plicate registrations)
|
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 9cfb445:
|
🕵 fluentuiv8 No visual regressions between this PR and main |
📊 Bundle size report🤖 This report was generated against 727a5dfc487ff37a26fa37226319dbe80eb02fcb |
Asset size changes
Baseline commit: 727a5dfc487ff37a26fa37226319dbe80eb02fcb (build) |
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| BaseButton | mount | 642 | 640 | 5000 | |
| Breadcrumb | mount | 1694 | 1663 | 1000 | |
| Checkbox | mount | 1702 | 1683 | 5000 | |
| CheckboxBase | mount | 1500 | 1478 | 5000 | |
| ChoiceGroup | mount | 2940 | 2900 | 5000 | |
| ComboBox | mount | 671 | 641 | 1000 | |
| CommandBar | mount | 6373 | 6333 | 1000 | |
| ContextualMenu | mount | 12437 | 12777 | 1000 | |
| DefaultButton | mount | 755 | 777 | 5000 | |
| DetailsRow | mount | 2167 | 2203 | 5000 | |
| DetailsRowFast | mount | 2179 | 2202 | 5000 | |
| DetailsRowNoStyles | mount | 2025 | 2021 | 5000 | |
| Dialog | mount | 2655 | 2669 | 1000 | |
| DocumentCardTitle | mount | 230 | 242 | 1000 | |
| Dropdown | mount | 1973 | 2021 | 5000 | |
| FocusTrapZone | mount | 1154 | 1141 | 5000 | |
| FocusZone | mount | 1070 | 1056 | 5000 | |
| GroupedList | mount | 41094 | 41338 | 2 | |
| GroupedList | virtual-rerender | 19913 | 19726 | 2 | |
| GroupedList | virtual-rerender-with-unmount | 50383 | 50606 | 2 | |
| GroupedListV2 | mount | 223 | 224 | 2 | |
| GroupedListV2 | virtual-rerender | 213 | 207 | 2 | |
| GroupedListV2 | virtual-rerender-with-unmount | 231 | 229 | 2 | |
| IconButton | mount | 1098 | 1071 | 5000 | |
| Label | mount | 347 | 346 | 5000 | |
| Layer | mount | 2769 | 2732 | 5000 | |
| Link | mount | 409 | 397 | 5000 | |
| MenuButton | mount | 963 | 955 | 5000 | |
| MessageBar | mount | 21655 | 21351 | 5000 | |
| Nav | mount | 1913 | 1951 | 1000 | |
| OverflowSet | mount | 778 | 791 | 5000 | |
| Panel | mount | 1822 | 1774 | 1000 | |
| Persona | mount | 744 | 763 | 1000 | |
| Pivot | mount | 874 | 869 | 1000 | |
| PrimaryButton | mount | 841 | 847 | 5000 | |
| Rating | mount | 4612 | 4642 | 5000 | |
| SearchBox | mount | 900 | 922 | 5000 | |
| Shimmer | mount | 1914 | 1922 | 5000 | |
| Slider | mount | 1363 | 1330 | 5000 | |
| SpinButton | mount | 2917 | 2830 | 5000 | |
| Spinner | mount | 381 | 390 | 5000 | |
| SplitButton | mount | 1817 | 1829 | 5000 | |
| Stack | mount | 405 | 402 | 5000 | |
| StackWithIntrinsicChildren | mount | 845 | 842 | 5000 | |
| StackWithTextChildren | mount | 2638 | 2584 | 5000 | |
| SwatchColorPicker | mount | 6227 | 6200 | 5000 | |
| TagPicker | mount | 1468 | 1539 | 5000 | |
| Text | mount | 371 | 368 | 5000 | |
| TextField | mount | 935 | 957 | 5000 | |
| ThemeProvider | mount | 833 | 837 | 5000 | |
| ThemeProvider | virtual-rerender | 587 | 580 | 5000 | |
| ThemeProvider | virtual-rerender-with-unmount | 1267 | 1250 | 5000 | |
| Toggle | mount | 605 | 615 | 5000 | |
| buttonNative | mount | 187 | 195 | 5000 |
khmakoto
approved these changes
Aug 31, 2023
marcosmoura
added a commit
to marcosmoura/fluentui
that referenced
this pull request
Sep 3, 2023
* master: (32 commits) refactor(react-drawer): change DrawerHeaderTitle slot creation while keeping the same API (microsoft#29042) test(react-drawer): add render tests for drawer subcomponents (microsoft#29043) Grouped vertical bar chart - Component tests (microsoft#29031) docs: add Fluent UI Insights EP06 to README (microsoft#29051) chore: use swc-plugin-de-indent-template-literal for consoles (microsoft#29040) chore: adds swc-plugin-de-indent-template-literal (microsoft#29037) feat(react-jsx-runtime): v9 packages to use importSource (microsoft#28959) chore: update swc build dependencies (microsoft#28989) fix(react-tags-preview): add hover/pressed style for windows high contrast (microsoft#29035) chore(react-tags-preview): use InteractionTag for TagGroup's stories (microsoft#29024) chore(react-tags-preview): use makeResetStyles for base styles (microsoft#29022) chore: fix codesandbox export for preview component by making @fluentui/react-components required dependency (microsoft#29016) applying package updates feat(react-motion): create useReducedMotion and apply to useMotion to skip animations (microsoft#29014) ReAdd: Keytips: Align keytipData with visible instance for dupes (microsoft#28992) feat(react-drawer): add motion to Drawer (microsoft#28999) fix(react-utilities): avoid calling requestAnimationFrame when in SSR (microsoft#29015) fix(ssr-tests-v9): use correct path for ssr-tests-v9 stories (microsoft#29025) chore: updates devcontainer to use v16-bookworm image (microsoft#28997) feat(docsite): add Application Insights telemetry (microsoft#28709) ...
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.
This PR is resubmitting #28522 with a slight alteration to only run querySelectorAll for keytips that are visible. This will greatly reduce the number of times querySelectorAll is run
_From the original PR:
Previous Behavior
Currently, keytips have the ability to render only for visible instances of target elements, but if there are multiple of the same registration (e.g. keytipData), the first one is always used. This can result in the keytip not being positioned correctly for the instance that's visible. Note, this is related to responsive scaling where the same control can be rendered with the same keytip tree multiple times where only one instance is visible at a time
New Behavior
With this change, the fetching of the keytipData will now return the instance that's associated with the corresponding visible element instance. This makes the keytipData that's used align with how keytips already choose where to be visible (e.g. which element is their target). This new code will only run if there are multiple keytips with the exact same registration (same keytip sequence (e.g. tree and element keytip)) which can happen for responsive scaling.