-
Notifications
You must be signed in to change notification settings - Fork 497
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
Invites are collapsed incorrectly #6300
Invites are collapsed incorrectly #6300
Conversation
- use sender avatar instead of the target avatar for collapsed membership cell
- fixed target ID for membership / join events
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 good to me, although I've left a question inline.
} | ||
else | ||
{ | ||
targetId = event.sender; |
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.
I'm not sure I understand why this part is necessary. If the cell now always shows the sender avatar, then how come the targetId
needs to be explicitly set to the sender here?
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.
You're right. I just found this bug while digging. I actually hesitated to add this fix and I can revert it if you want.
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.
I'm not very sure what the issue requested with and the avatars should show the invited user not the invitee. “2 users invited” would be correct.
I believe he wanted to have invited user instead of the inviter. If that assumption is correct, i think we'll need this part? (and keep using targetId
for avatars)
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.
@ismailgulek I agree. I've just checked with android people and they confirmed the behaviour is the same. I'll check with the design team in this case.
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.
@ismailgulek I've checked with @amshakal and we'll display the avatar of the invitee in the collapsed membership cell even if it's a bit inconsistent with the expanded membership.
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.
Great, thanks for checking.
📱 Scan the QR code below to install the build for this PR. If you can't scan the QR code you can install the build via this link: https://i.diawi.com/s4ZoZS |
Co-authored-by: ismailgulek <ismailgulek@users.noreply.github.com>
- Update after review
* Invites are collapsed incorrectly - use invitee avatar instead of the target avatar for collapsed membership cell
resolves #4102
Now use sender avatar instead of the target avatar for collapsed membership cell