feat(tooltip)!: add post-tooltip-trigger #3196
Conversation
In order to get rid of mutation observers on attributes, a new element to wrap the trigger was added for the tooltip. This enables easier lifecycle management and more precise event listeners with less overhead.
🦋 Changeset detectedLatest commit: 268fda3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
In order to hide the post-tooltip with display: none initially, an open attribute is reflected to the custom element. This enables to set display: block when the attribute is present, handling issues with screen readers.
packages/components/src/components/post-tooltip-trigger/post-tooltip-trigger.tsx
Show resolved
Hide resolved
| this.trigger.setAttribute('aria-describedby', newDescribedBy); | ||
| } | ||
|
|
||
| this.host.addEventListener('pointerover', this.localInterestHandler); |
There was a problem hiding this comment.
Why not adding the event listeners on the trigger instead of the host?
There was a problem hiding this comment.
Just in case the trigger gets replaced or re-rendered by react/angular in which case the event bindings would be lost. If the trigger is re-rendered, the lifecycle hooks will trigger again.
There was a problem hiding this comment.
True, in this case the ARIA attributes will not be set properly. We might need a Mutation observer in the end...
packages/components/src/components/post-tooltip-trigger/post-tooltip-trigger.tsx
Outdated
Show resolved
Hide resolved
packages/components/src/components/post-tooltip-trigger/post-tooltip-trigger.tsx
Outdated
Show resolved
Hide resolved
…ooltip-trigger.tsx Co-authored-by: Alizé Debray <[email protected]>
…ooltip-trigger.tsx Co-authored-by: Alizé Debray <[email protected]>
|
| /** | ||
| * Link the trigger to a tooltip with this id | ||
| */ | ||
| @Prop() for: string; |
There was a problem hiding this comment.
Validation is missing, checkNonEmpty at least.
|
|
||
| // Add tooltip to aria-describedby | ||
| const describedBy = this.trigger.getAttribute('aria-describedby'); | ||
| if (!describedBy?.includes(this.for)) { |
There was a problem hiding this comment.
It would be good to move this logic to the "for" watcher in case it changes after the component load.
| this.trigger.setAttribute('aria-describedby', newDescribedBy); | ||
| } | ||
|
|
||
| this.host.addEventListener('pointerover', this.localInterestHandler); |
There was a problem hiding this comment.
True, in this case the ARIA attributes will not be set properly. We might need a Mutation observer in the end...
| @@ -181,7 +62,7 @@ export class PostTooltip { | |||
|
|
|||
| /** | |||
| * Toggle tooltip display | |||
| * @param target An element with [data-tooltip-target="id"] where the tooltip should be shown | |||
| * @param target An element where the tooltip should be shown | |||
| * @param force Pass true to always show or false to always hide | |||
There was a problem hiding this comment.
| * @param force Pass true to always show or false to always hide | |
| * @param [force] Pass true to always show or false to always hide |
| /** | ||
| * Indicates the open state of the tooltip | ||
| */ | ||
| @Prop({ reflect: true, mutable: true }) open = false; |
There was a problem hiding this comment.
Props validation is missing, they should also be added to the changeset.
There was a problem hiding this comment.
These changes can probably be reverted.
| /** | ||
| * Animation style | ||
| */ | ||
| @Prop() readonly animation?: 'pop-in' = null; |
There was a problem hiding this comment.
| @Prop() readonly animation?: 'pop-in' = null; | |
| @Prop() readonly animation?: 'pop-in'; |
OR
| @Prop() readonly animation?: 'pop-in' = null; | |
| @Prop() readonly animation: 'pop-in' | null = null; |
| @@ -62,11 +62,17 @@ const meta: MetaComponent = { | |||
| placement: { | |||
| name: 'Placement', | |||
| }, | |||
| arrow: { | |||
| name: 'Arrow', | |||
| animation: { | |||
There was a problem hiding this comment.
Why did you override the animation arg?
It would be better to use the definition derived from the JSDoc of the component.
In order to get rid of mutation observers on attributes, a new element to wrap the trigger was added for the tooltip. This enables easier lifecycle management and more precise event listeners with less overhead.