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

fix(LeftSidebar): wrong user status after scrolling #10356

Merged
merged 1 commit into from
Aug 25, 2023

Conversation

ShGKme
Copy link
Contributor

@ShGKme ShGKme commented Aug 24, 2023

☑️ Resolves

On scrolling user status in 1-1 conversations in the LeftSidebar is not correct.

  • User's avatar and user status is displayed by NcAvatar component.
  • With virtual scrolling components are reused. During scrolling new visible conversations are just given props of previous.
  • It works only for components that react on props update
  • NcAvatar supports user update but doesn't support preloadedUserStatus update

🚧 Tasks

  • Add key to NcAvatar and re-mount avatar for 1-1 conversation during scrolling.

Drawbacks and alternative solution

It is less performant that adding support of preloadUserStatus change to NcAvatar.

Ideally it should be fixed on @nextcloud/vue. But I cannot say how much is the difference.

🏁 Checklist

Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
@ShGKme
Copy link
Contributor Author

ShGKme commented Aug 24, 2023

/backport to stable27

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.

Tested with desktop client on c.nc.c, all statuses are at their own places.

follow-up:

  • does it make sense to keep img tag now?
    image

  • can we upstream handling of preloadedUserStatus? watch for prop changes to update should be enough, I suppose?

@Antreesy Antreesy merged commit 28c757f into master Aug 25, 2023
@Antreesy Antreesy deleted the fix/left-sidebar-wrong-user-status-on-scrolling branch August 25, 2023 12:37
@ShGKme
Copy link
Contributor Author

ShGKme commented Aug 25, 2023

does it make sense to keep img tag now?

key on <img> helps to keeps Image DOM element during scrolling and re-create it on long scrolling when src actually changes. It removes glitch when src is updated on existing image.

key on <NcAvatar> recreates a whole component instance. It also re-creates its <img>. But I'd call it a temporal solution. We should not re-create <NcAvatar>.

So we should add status update support to <NcAvatar> and check if there is anything we can disable when it is not needed for performant update.

We can also consider adding key to img inside NcAvatar. But need to check if it won't have any drawbacks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants