Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Sep 17, 2024

Conversation

susnux
Copy link
Contributor

@susnux susnux commented Sep 9, 2024

☑️ 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

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

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

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
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?

src/components/NcActions/NcActions.vue Outdated Show resolved Hide resolved
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 <opensource@fthiessen.de>
@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
19 checks passed
@susnux susnux deleted the fix/nc-actions-closed branch September 17, 2024 08:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement New feature or request feature: actions Related to the actions components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants