Tooltips fixes #12067
Tooltips fixes #12067
Conversation
vitvakatu
requested review from
Frizi,
farmaazon,
kazcw and
AdRiley
as code owners
🧪 Storybook is successfully deployed!📊 Dashboard:
|
| @@ -13,14 +14,18 @@ import TooltipTrigger from '@/components/TooltipTrigger.vue' | |||
|
|
|||
| const toggledOn = defineModel<boolean>({ default: undefined }) | |||
| const props = defineProps<{ disabled?: boolean | undefined; title?: string | undefined }>() | |||
| const tooltipTrigger = ref<typeof TooltipTrigger>() | |||
There was a problem hiding this comment.
Wait, so you don't need ComponentInstance or ComponentExposed here?
There was a problem hiding this comment.
Well, not really, tsc is happy 🤷🏻♂️ I added just for consistency.
| if ( | ||
| previousTooltip.value != null && | ||
| !previousTooltip.value.entry.isHidden && | ||
| previousTooltip.value.element.isConnected | ||
| ) { | ||
| return previousTooltip.value | ||
| } |
There was a problem hiding this comment.
Please add a comment explaining why we sometimes display the tooltip at the previous element - I think it's not obvious without the context of the bug you're fixing.
| if (entriesSet) { | ||
| for (const e of entriesSet) { | ||
| if (e.key === key) entriesSet.delete(e) | ||
| } | ||
| } |
There was a problem hiding this comment.
We may have quite a lot of entries in this set - could it be a map?
Also, why didn't the previous approach work? In general I feel a bit more documentation would be helpful; for example I don't get why hoveredElements is map of HTMLElement -> Set - do we have many tooltips registered for single HTML element?
There was a problem hiding this comment.
The set is usually just one element long. But in case of nested tooltip triggers (which are extremely rare) it can be longer.
Pull Request Description
Closes #10999
Fixes three issues with tooltips:
tooltips.mp4
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
or the Snowflake database integration, a run of the Extra Tests has been scheduled.