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

Follow-up: chat scrolling refactoring #11943

Merged
merged 4 commits into from
Apr 23, 2024

Conversation

DorraJaouad
Copy link
Contributor

@DorraJaouad DorraJaouad commented Mar 27, 2024

☑️ Resolves

TO-DO

  • 👷🏻‍♀️ Refactor
  • ⚠️ Fix conversation jumping on opening
  • 🔴 Empty content is not shown anymore because of loadedMessages flag does not change to true in empty conv
  • 💚 Addition: add a floating button at the bottom in case you are scrolled up and new messages come in ( aka indicator for new messages while opening the chat in top)

🖌️ UI Checklist

🖼️ Screenshots / Screencasts

Conversation switching before

Recording.2024-04-19.before.mp4

Conversation switching after

Recording.2024-04-19.after.mp4

🏁 Checklist

  • 🌏 Tested with Chrome, Firefox and Safari or should not be risky to browser differences
  • 🖥️ Tested with Desktop client or should not be risky for it
  • 🖌️ 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

@DorraJaouad DorraJaouad self-assigned this Mar 27, 2024
@DorraJaouad DorraJaouad added this to the 💙 Next Major (30) milestone Mar 27, 2024
@DorraJaouad
Copy link
Contributor Author

DorraJaouad commented Apr 15, 2024

ChatIdentifier is updated on two steps:

  1. {token: 't3qj6xtm', isParticipant: false, isInLobby: false}
    Chat is not scrolled yet because , the condition in handleStartGettingMessagesPreconditions is faulty
  2. {token: 't3qj6xtm', isParticipant: true, isInLobby: false}
    this is when the chat is finally scrolled

isParticipant is not updated instantly, need to find workarounds to make it available

UPD; or just ignore it :p. It is not needed in handling opening conversations. If the user is actually not a participant, it seems blocked by API. --> dropped this idea because it might lead to brute force protection triggered.

UPD2: used conversation store, as it is instantly available

@DorraJaouad DorraJaouad force-pushed the fix/noid/refactor-chat-scrolling branch from 1ee802a to 0c0a82c Compare April 15, 2024 13:22
@DorraJaouad DorraJaouad requested a review from Antreesy April 15, 2024 13:24
@DorraJaouad DorraJaouad marked this pull request as ready for review April 15, 2024 17:01
@DorraJaouad
Copy link
Contributor Author

DorraJaouad commented Apr 15, 2024

Focused message from URL doesn't focus correctly sometimes 🥲
--> Fixed

@DorraJaouad DorraJaouad force-pushed the fix/noid/refactor-chat-scrolling branch from 6132f8e to 624ebf1 Compare April 16, 2024 07:57
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.

Checked only the last commits (unrelated to scrolling logic):

src/store/participantsStore.js Outdated Show resolved Hide resolved
src/store/participantsStore.js Show resolved Hide resolved
src/components/MessagesList/MessagesList.vue Outdated Show resolved Hide resolved
src/store/messagesStore.js Show resolved Hide resolved
@DorraJaouad DorraJaouad force-pushed the fix/noid/refactor-chat-scrolling branch from 624ebf1 to e7cebc2 Compare April 16, 2024 19:23
src/store/participantsStore.js Outdated Show resolved Hide resolved
src/components/MessagesList/MessagesList.vue Outdated Show resolved Hide resolved
src/components/MessagesList/MessagesList.vue Show resolved Hide resolved
@DorraJaouad DorraJaouad force-pushed the fix/noid/refactor-chat-scrolling branch from e7cebc2 to a325ae0 Compare April 19, 2024 12:51
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.

As discussed during pair-review, seems to work much better. Seems that even the opening of every chat at the top is resolved! One more thing to try to reproduce and fix:

src/components/MessagesList/MessagesList.vue Outdated Show resolved Hide resolved
src/components/MessagesList/MessagesList.vue Show resolved Hide resolved
@Antreesy
Copy link
Contributor

I'm quite impressed with the final result, to be honest =P

cc @nickvergessen @ShGKme @danxuliu : I'd suggest to look through it once more thoroughly (maybe after RC), and at best - compile the desktop client with this PR and use it for a while. If we consider it good enough, then could backport for 19.0.1

Signed-off-by: DorraJaouad <dorra.jaoued7@gmail.com>
Signed-off-by: DorraJaouad <dorra.jaoued7@gmail.com>
Signed-off-by: DorraJaouad <dorra.jaoued7@gmail.com>
Signed-off-by: DorraJaouad <dorra.jaoued7@gmail.com>
@DorraJaouad DorraJaouad force-pushed the fix/noid/refactor-chat-scrolling branch from a325ae0 to a731695 Compare April 19, 2024 16:46
@DorraJaouad DorraJaouad merged commit 8f7b181 into main Apr 23, 2024
45 checks passed
@DorraJaouad DorraJaouad deleted the fix/noid/refactor-chat-scrolling branch April 23, 2024 11:42
@DorraJaouad
Copy link
Contributor Author

/backport to stable29

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

Successfully merging this pull request may close these issues.

2 participants