Add new member after typing message and pressing enter #17014
Conversation
jrainville
requested review from
micieslak,
caybro,
alexjba and
a team
as code owners
Jenkins BuildsClick to see older builds (7)
|
jrainville
force-pushed
the
feat/group-chat-add-member-improvement
branch
from
0487ddc to
3fb89d6
Compare
| @@ -291,6 +292,12 @@ StatusSectionLayout { | |||
| amISectionAdmin: root.amISectionAdmin | |||
| sendViaPersonalChatEnabled: root.sendViaPersonalChatEnabled | |||
| paymentRequestFeatureEnabled: root.paymentRequestFeatureEnabled | |||
| selectingMembers: headerContentLoader.item && !!headerContentLoader.item.selectingMembers | |||
| onGroupUpdated: { | |||
| if (headerContentLoader.item && headerContentLoader.item instanceof ChatHeaderContentView) { | |||
There was a problem hiding this comment.
This seems somewhat odd... to rely on the concrete underlying type of the component; but I don't really have a better idea atm 🤷
There was a problem hiding this comment.
Yeah, it was either that or doing !!headerContentLoader.item.groupUpdated. It's as you prefer
There was a problem hiding this comment.
Is this condition needed at all? It's already guarded by !!headerContentLoader.item.selectingMembers which will be evaluated to false when it's not instance of ChatHeaderContentView, preventing emiting of that signal.
Fixes #8349 When the member selector menu is open, sending a message adds/removes the members and then sends the message. Therefore, the new member will also receive the message.
jrainville
force-pushed
the
feat/group-chat-add-member-improvement
branch
from
3fb89d6 to
0702919
Compare
|
@alexjba @micieslak friendly reminder to review |
There was a problem hiding this comment.
Generally looks ok and works, but I proposed some improvements regarding the architecture to reduce coupling.
Basically the involved components seem to be highly over-complicated, with no good separation of concerns and tightly coupled. Probably we should find space to start refactoring this area step-by-step.
| // If the member selector is open, we update the group members and call for the menu to be closed | ||
| if (selectingMembers) { | ||
| d.activeUsersStore.updateGroupMembers() | ||
| root.groupUpdated() | ||
| } |
There was a problem hiding this comment.
Basically, it shouldn't be responsibility of that component to care about state of some other components and call additional actions there.
To achieve that we could add signal messageAboutToBeSent at top level and emit it here.
Than we don't care about the custom external logic. We don't need groupUpdated signal and selectingMembers flag. That logic can be handled by the user of that component by hooking to to onMessageAboutToBeSent signal.
| @@ -291,6 +291,12 @@ StatusSectionLayout { | |||
| amISectionAdmin: root.amISectionAdmin | |||
| sendViaPersonalChatEnabled: root.sendViaPersonalChatEnabled | |||
| paymentRequestFeatureEnabled: root.paymentRequestFeatureEnabled | |||
| selectingMembers: headerContentLoader.item && !!headerContentLoader.item.selectingMembers | |||
| onGroupUpdated: { | |||
There was a problem hiding this comment.
instead of selectingMembers and onGroupUpdated we can handle the proposed onMessageAboutToBeSent:
onMessageAboutToBeSent: {
if (!headerContentLoader.item || !headerContentLoader.item.selectingMembers)
return
// ...
}
| @@ -291,6 +292,12 @@ StatusSectionLayout { | |||
| amISectionAdmin: root.amISectionAdmin | |||
| sendViaPersonalChatEnabled: root.sendViaPersonalChatEnabled | |||
| paymentRequestFeatureEnabled: root.paymentRequestFeatureEnabled | |||
| selectingMembers: headerContentLoader.item && !!headerContentLoader.item.selectingMembers | |||
| onGroupUpdated: { | |||
| if (headerContentLoader.item && headerContentLoader.item instanceof ChatHeaderContentView) { | |||
There was a problem hiding this comment.
Is this condition needed at all? It's already guarded by !!headerContentLoader.item.selectingMembers which will be evaluated to false when it's not instance of ChatHeaderContentView, preventing emiting of that signal.
What does the PR do
Fixes #8349
When the member selector menu is open, sending a message adds/removes the members and then sends the message. Therefore, the new member will also receive the message.
The first commit is just a cleanup of code taht is never used
Affected areas
Group chat member selection (for existing chat)
Architecture compliance
My PR is consistent with this document: Status Desktop Architecture Guide
Screenshot of functionality (including design for comparison)
add-remove-members.webm
Impact on end user
Enables the user to add or remove members more easily
How to test
Also, using the Confirm as before works as usual
Risk
Tick one:
Worst case the new feature doesn't work