-
Notifications
You must be signed in to change notification settings - Fork 434
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(MessageGroup) - group system messages #9777
Conversation
src/components/MessagesList/MessagesGroup/MessagesSystemGroup.vue
Outdated
Show resolved
Hide resolved
279a7c7
to
3d7f282
Compare
1d53c0f
to
0f7d991
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice, could we please always align the button with the date? Meaning to the first line of the system message
src/components/MessagesList/MessagesGroup/MessagesSystemGroup.vue
Outdated
Show resolved
Hide resolved
follow-up: messages are re-rendered on each fetch, so hidden messages collapse each time |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice @Antreesy! One detail – as per design guidelines we never use italic font as it can be difficult to read for people. Since they are in a container already they do not need to be italic as well. :)
571eb99
to
53fc88d
Compare
Rebased on top of refactor+bugfix PR #9897 |
53fc88d
to
209a8ec
Compare
Is it still work in progress, or ready for code review? |
There still is a room for improvement, but mostly about error cleanup and refactoring. |
209a8ec
to
098c6da
Compare
Maybe underlying "and 7 more participants" and making it into an |
Looks really really nice! :)
If there are no a11y issues with this, then this sounds great! This would be the ideal solution. Maybe @Pytal or @JuliaKirschenheuter could help with figuring out if showing the button only on hover is okay? :)
I'm not too sure about this as it's not really a link at all, and we rarely (if at all) use link styling to indicate other actions. |
Hiding the action to reduce visual clutter sounds good but make sure that it is tabbable and visible when focused/hovered |
@nimishavijay @marcoambrosini stylized toggle button as P.S. Updated screenshots in description |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really nice! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to works fine. Nothing blocking, but some naming/architecture comments
|
…equently Signed-off-by: Maksim Sukharev <[email protected]>
Signed-off-by: Maksim Sukharev <[email protected]>
81c6d9b
to
1dfc54b
Compare
Rebased onto master |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested manually in basic cases ✅
These components and grouping logic has many edge cases. IMO, we should cover it with tests.
<div v-for="messagesCollapsed of messagesGroupedBySystemMessage" | ||
:key="messagesCollapsed.id" | ||
class="message-group__system"> | ||
<ul v-if="messagesCollapsed.messages?.length > 1" | ||
class="messages messages--header"> | ||
<Message v-bind="createCombinedSystemMessage(messagesCollapsed)" | ||
is-combined-system-message | ||
:is-combined-system-message-collapsed="messagesCollapsed.collapsed" | ||
:next-message-id="getNextMessageId(messagesCollapsed.messages.at(-1))" | ||
:previous-message-id="getPrevMessageId(messagesCollapsed.messages.at(0))" | ||
@toggle-combined-system-message="toggleCollapsed(messagesCollapsed)" /> | ||
</ul> | ||
<ul v-show="messagesCollapsed.messages?.length === 1 || !messagesCollapsed.collapsed" | ||
class="messages" | ||
:class="{'messages--collapsed': messagesCollapsed.messages?.length > 1}"> | ||
<Message v-for="message of messagesCollapsed.messages" | ||
:key="message.id" | ||
v-bind="message" | ||
:next-message-id="getNextMessageId(message)" | ||
:previous-message-id="getPrevMessageId(message)" /> | ||
</ul> | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This component has many methods calls in template (that a called during the re-rendering) and those methods are not small utils or formatting methods but data processing methods.
It won't be a problem if it doesn't have unnecessary re-renderings, but currently MessagesList
re-renders each new message.
For example, a user has a chat with mostly only calls, but large calls, for example, 100 people. Then in the history there will be many groups with 100 system messages about leaving the call. And every message, reaction, poll update will re-render the list with recalculating all these methods looping over 100 messages for each group.
IMO, we can keep it in this way but then do one of:
- Find a reason of re-renderings
- Refactor all these components, remove unnecessary props
- Remove these calculating inside rendering
- Wait for Vue 3 migration =D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I'd prefer to use in
in v-for
directives to use only one style. (v-for of
is not very often used in our components).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"of" is replaced with "in". Test coverage and components re-rendering moved to follow-up
Signed-off-by: Maksim Sukharev <[email protected]>
Signed-off-by: Maksim Sukharev <[email protected]>
Signed-off-by: Maksim Sukharev <[email protected]>
Signed-off-by: Maksim Sukharev <[email protected]>
…first message line Signed-off-by: Maksim Sukharev <[email protected]>
Signed-off-by: Maksim Sukharev <[email protected]>
…r.vue Signed-off-by: Maksim Sukharev <[email protected]>
1dfc54b
to
9b1c9d0
Compare
Blocking question has been resolved
/backport to stable27 |
☑️ Resolves
user added/removed
=>moderator promoted/demoted
=>call started
=>recording started
=>call joined/left
=>call ended
🖼️ Screenshots
🚧 Tasks
call_left
andcall_joined
in various combinations, so they're not sorted between each other)🏁 Checklist
docs/
has been updated or is not required