feat(components): updated the post-popover-trigger to enable the option of wrapping the popover #6626
Conversation
🦋 Changeset detectedLatest commit: 771663d 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 |
…-the-popover-to-omit-the-id-reference
leagrdv
left a comment
leagrdv
left a comment
There was a problem hiding this comment.
Could this also be done for the tooltip? Maybe in another ticket
There was a problem hiding this comment.
Since there is no "for", the aria-controls is set to undefined
There was a problem hiding this comment.
Correct! I ve updated that to either use the Elements API ariaControlsElements property, or fallback to a random ID generation.
| @Watch('for') | ||
| validateFor() { | ||
| checkRequiredAndType(this, 'for', 'string'); | ||
| checkEmptyOrType(this, 'for', 'string'); |
There was a problem hiding this comment.
Using a post-popovertrigger without a for or a wrapped post-popover shows no error (just a warning when clicking on the trigger). There should be a check that there is either one or the other and show an error if that's not the case.
Could you also add some tests for this?
…-the-popover-to-omit-the-id-reference
There was a problem hiding this comment.
Just a hint...
When the scope of a test is very limited (e.g. we're testing one specific component), it is often easier, to keep the names of the aliases short, because you have to repeat them a lot afterwards.
Try to be consisting with the alias naming.
beforeEach(() => {
cy.get('post-popover[data-hydrated][id="popover-one"]').as('popoverOne');
cy.get('@popoverOne').find('p').as('contentOne');
cy.get('post-popover-trigger[data-hydrated][for="popover-one"]').as('triggerOne');
cy.get('@triggerOne').find('button').as('buttonOne');
});
packages/components/src/components/post-popover-trigger/post-popover-trigger.tsx
Outdated
Show resolved
Hide resolved
packages/components/src/components/post-popover-trigger/post-popover-trigger.tsx
Show resolved
Hide resolved
| // Set aria attributes | ||
| this.trigger.setAttribute('aria-haspopup', 'true'); | ||
| this.trigger.setAttribute('aria-controls', this.for); | ||
| const popover = this.popover; |
There was a problem hiding this comment.
As a general rule, declare a const as early as possible in its scope!
And no worry, it has no effect to the memory usage even if you don't use it afterwards, as const popover is a simple reference to the same object (memory address) as this.popover (as long as this.popover does not return a primitive like null or a string).
This is considered good practice.
There was a problem hiding this comment.
Good to know, thanx!
| popover.id ||= `popover-${crypto.randomUUID()}`; | ||
| (this.trigger as HTMLElement).setAttribute('aria-controls', popover.id); |
There was a problem hiding this comment.
Why is the type assertion necessary here, but not on line 93?
There was a problem hiding this comment.
It happens because of the else if ('ariaControlsElements' in this.trigger) check, ts sees it as never. I updated with a helper function.
Co-authored-by: Oliver Schürch <[email protected]>
|
📄 Description
This PR updates the
post-popover-triggerto enable the option of wrapping thepost-popoverinside it and omitting the id reference.🔮 Design review
📝 Checklist