Skip to content

Conversation

@Antreesy
Copy link
Contributor

@Antreesy Antreesy commented Jun 23, 2025

☑️ Resolves

  • Fix leaking component data
  • passing key forces component to rerender (if submenu page was changed)
    • before close-after-click could be leaked as true to a new component, which should not have it

🖌️ UI Checklist

A B
2025-06-25_09h24_48 2025-06-25_09h25_01

🚧 Tasks

  • Alternative: check it in vue-library instead?

🏁 Checklist

  • 🌏 Tested with different browsers / clients:
    • Chromium (Chrome / Edge / Opera / Brave)
    • Firefox
    • Safari
    • Talk Desktop
    • Not risky to browser differences / client
  • 🖌️ Design was reviewed, approved or inspired by the design team
  • ⛑️ Tests are included or not possible
  • 📗 User documentation in https://github.com/nextcloud/documentation/tree/master/user_manual/talk has been updated or is not required

<template v-else-if="submenu === 'notifications'">
<NcActionButton :aria-label="t('spreed', 'Back')"
<NcActionButton
key="action-back"
Copy link
Member

Choose a reason for hiding this comment

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

does this refresh good enough even when its reused inside a parent that has a different key? Just asking for my understanding

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if parent has a different key, it should re-render parent and everything underneath

Copy link
Contributor

Choose a reason for hiding this comment

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

does this refresh good enough even when its reused inside a parent that has a different key? Just asking for my understanding

Key doesn't trigger re-rendring, but re-order/re-mount.

And Vue doesn't support parent remounting without children remounting.

So changing the parent key completely destroys and re-creates children. Not just re-render.

Copy link
Contributor

@DorraJaouad DorraJaouad left a comment

Choose a reason for hiding this comment

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

Nearest parent is NcListItem (skipping templates) and it has key updated by token so it should be enough?

@Antreesy
Copy link
Contributor Author

Antreesy commented Jun 25, 2025

Nearest parent is NcListItem (skipping templates) and it has key updated by token so it should be enough?

It's not about Virtual scroller, but switching submenus (see above)

Copy link
Contributor

@ShGKme ShGKme left a comment

Choose a reason for hiding this comment

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

  • passing key forces component to rerender (if submenu page was changed)

Rerendering is triggered by reactive data change.
If a component doesn't rerender on a state change, there is a reactivity loss, and it should be fixed.

key either help Vue to identify an instance and reorder instances instead of updating props of instances (v-for case), or triggers remount (recreate) of a component instance in case of a completely new key.

NcActionButton supports updating props even in a v-for without key.
But probably something goes wrong due to our manual rendering in NcActions.

These keys don't hurt (they don't create unneeded remounts), let's go with them.

@Antreesy Antreesy merged commit 32ad805 into vue3 Jun 25, 2025
49 of 51 checks passed
@Antreesy Antreesy deleted the fix/vue3/keys branch June 25, 2025 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants