feat(components): enable open animation on <post tooltip>
#6821
Conversation
🦋 Changeset detectedLatest commit: de27e1d 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 |
…tooltip-and-popover
myrta2302
changed the base branch from
main
to
6538-bug-fix-post-menu-animation
…ot-actually-applied-to-post-tooltip-and-popover
…tooltip-and-popover
…-popover' of https://github.com/swisspost/design-system into 6617-animation-not-actually-applied-to-post-tooltip-and-popover
|
myrta2302
requested review from
leagrdv
and removed request for
a team and
leagrdv
leagrdv
requested review from
oliverschuerch
and removed request for
leagrdv
| @Method() | ||
| async toggle(target: HTMLElement, force?: boolean) { | ||
| await this.popoverRef.toggle(target, force); | ||
| this.focusFirstEl(); |
There was a problem hiding this comment.
Can we avoid calling this function when we toggle to false?
There was a problem hiding this comment.
I 'm not sure. I had already thought about it, but currently post-menu toggle() calls it like this - without the force: this.popoverRef.toggle(target). Would you prefer updating the post-menu?
There was a problem hiding this comment.
No, that was not my intention, but it would be good if it does the exact same thing.
I just had a look into your PR and I think we can do the following to optimize this functionality:
async toggle(target: HTMLElement, force?: boolean) {
// you can expect the popoverRef.toggle function to return the current state (after toggling), no matter if we call it with a `force` parameter or not ;)
const isOpen = await this.popoverRef.toggle(target, force);
if (isOpen) this.focusFirstEl();
}This is only a micro improvement reguarding performance, but it could save us from unwanted side-effects.
| @Method() | ||
| async toggle(target: HTMLElement, force?: boolean) { | ||
| await this.popoverRef.toggle(target, force); | ||
| this.focusFirstEl(); |
There was a problem hiding this comment.
No, that was not my intention, but it would be good if it does the exact same thing.
I just had a look into your PR and I think we can do the following to optimize this functionality:
async toggle(target: HTMLElement, force?: boolean) {
// you can expect the popoverRef.toggle function to return the current state (after toggling), no matter if we call it with a `force` parameter or not ;)
const isOpen = await this.popoverRef.toggle(target, force);
if (isOpen) this.focusFirstEl();
}This is only a micro improvement reguarding performance, but it could save us from unwanted side-effects.
myrta2302
changed the title
<post tooltip><post tooltip>
myrta2302
changed the base branch from
6538-bug-fix-post-menu-animation
to
main
📄 Description
This PR updates
<post-tooltip>so that it now correctly uses the open animation provided by<post-popovercontainer>, which was previously not visible.Additionally, but not directly connected to the scope of the current ticket it:
<post-popover>component. (Popover animation has already been enabled in the [bug] fix post-menu animation #6538 )🔮 Design review
📝 Checklist