Skip to content

Comments

feat(NcActions): Emit closed event only when the actions are fully closed#6065

Merged
susnux merged 1 commit intomasterfrom
fix/nc-actions-closed
Sep 17, 2024
Merged

feat(NcActions): Emit closed event only when the actions are fully closed#6065
susnux merged 1 commit intomasterfrom
fix/nc-actions-closed

Conversation

@susnux
Copy link
Contributor

@susnux susnux commented Sep 9, 2024 •
edited
Loading

☑️ Resolves

The flow is like:

  1. Close the actions
  2. update:open with false is emitted
  3. Actions popover closes (transition)
  4. close is emitted

Fixes flashing changes in the menu if you have conditionals like submenus.

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable
  • 3️⃣ Backport to next requested with a Vue 3 upgrade

@susnux susnux added bug Something isn't working 3. to review Waiting for reviews feature: actions Related to the actions components labels Sep 9, 2024
@skjnldsv
Copy link
Contributor

skjnldsv commented Sep 11, 2024

What about close/closed ?

I usually prefer to have both options when there is an animation :)

@susnux
Copy link
Contributor Author

susnux commented Sep 11, 2024

What about close/closed ?

The "state changed but still visible" event is already available with update:open, the documentation was implicating that the event (close) is emitted when it is closed, also using apps logic (from what I understand having a quick search) were expecting this event to be emitted when the menu is closed (not closing).

So if you need the closing state it would be listening for ('update:open', false).

Maybe we should rename the event in the next major to closed ? Or add the closed event right now and deprecate close?

@skjnldsv
Copy link
Contributor

skjnldsv commented Sep 11, 2024

Maybe we should rename the event in the next major to closed ? Or add the closed event right now and deprecate close?

Or keep both 🙈

@ShGKme
Copy link
Contributor

ShGKme commented Sep 12, 2024

Or keep both 🙈

Only if one is deprecated in v8 and removed in v9. Otherwise why have 2 names for an event?

@skjnldsv
Copy link
Contributor

skjnldsv commented Sep 12, 2024

update:open is a bit of a different event type I'd say.

@susnux susnux force-pushed the fix/nc-actions-closed branch from 5bb6b1d to f17f588 Compare September 16, 2024 11:28
@susnux susnux changed the title fix(NcActions): Emit close event only when the actions are fully closed feat(NcActions): Emit closes event only when the actions are fully closed Sep 16, 2024
@susnux susnux changed the title feat(NcActions): Emit closes event only when the actions are fully closed feat(NcActions): Emit closed event only when the actions are fully closed Sep 16, 2024
@susnux susnux added enhancement New feature or request and removed bug Something isn't working labels Sep 16, 2024
@susnux susnux mentioned this pull request Sep 16, 2024
Merged
Antreesy
Antreesy approved these changes Sep 16, 2024
View reviewed changes
Copy link
Contributor

@Antreesy Antreesy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point
We have the same events firing at NcAppSidebar, should be noted/fixed there as well?

@susnux @mejo-
feat(NcActions): Emit closed event when the actions are fully closed
c729694 
The flow is like:
1. Close the actions
2. `update:open` with `false` is emitted (+ deprecated 'close' event)
3. Actions popover closes (transistion)
4. `closed` is emitted

Signed-off-by: Ferdinand Thiessen <[email protected]>
@mejo- mejo- force-pushed the fix/nc-actions-closed branch from f17f588 to c729694 Compare September 17, 2024 08:04
@susnux susnux merged commit e2018a1 into master Sep 17, 2024
@susnux susnux deleted the fix/nc-actions-closed branch September 17, 2024 08:07
@skjnldsv skjnldsv mentioned this pull request Apr 2, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mejo- mejo- mejo- left review comments

@skjnldsv skjnldsv skjnldsv approved these changes

@ShGKme ShGKme ShGKme approved these changes

@Antreesy Antreesy Antreesy approved these changes

@raimund-schluessler raimund-schluessler Awaiting requested review from raimund-schluessler

Assignees

No one assigned

Labels

3. to review Waiting for reviews enhancement New feature or request feature: actions Related to the actions components

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@susnux @skjnldsv @ShGKme @mejo- @Antreesy