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.

Sign up for GitHub

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

Jump to bottom

[MM-53118] Dynamically sized participants grid #805

Merged
merged 9 commits into from
Aug 14, 2024
Merged

[MM-53118] Dynamically sized participants grid #805

merged 9 commits into from
Aug 14, 2024

Conversation

streamer45
Copy link
Collaborator

Summary

PR takes a first pass at implementing a dynamically sized call participants grid in the pop-out window view.

Specs

https://mattermost.atlassian.net/wiki/spaces/Calls/pages/2707193901/Calls+Participant+Grid
https://www.figma.com/design/8kcq0HRN2TUaVAdPeeSvsb/Calls-Participants-Grid?node-id=1-7&m=dev

Video

dynamic_grid.mp4

Ticket Link

https://mattermost.atlassian.net/browse/MM-53118

@streamer45 streamer45 added 1: UX Review Requires review by ux 2: Dev Review Requires review by a core committer labels Jul 2, 2024
@streamer45 streamer45 self-assigned this Jul 2, 2024
streamer45
streamer45 commented Jul 2, 2024
View reviewed changes
webapp/src/websocket_handlers.ts
@@ -185,7 +185,7 @@ export function handleUserJoined(store: Store, ev: WebSocketMessage<UserJoinedDa
}

if (ringingEnabled(store.getState()) && userID === currentUserID) {
const callID = calls(store.getState())[channelID].ID || '';
const callID = calls(store.getState())[channelID]?.ID || '';
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's unrelated, but I spotted a crash, so I'm fixing it here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yikes, thanks.

cpoile
cpoile approved these changes Jul 3, 2024
View reviewed changes
Copy link
Member

@cpoile cpoile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! I'll assume the math checks out so I don't have to write it out myself. :P And I'll leave the UI/UX to Abhijit.
Thanks!

webapp/src/components/expanded_view/call_participant.tsx
@@ -31,8 +39,52 @@ export type Props = {
isSharingScreen?: boolean,
}

const tileSizePropsMap = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is nice and clean, good idea.

webapp/src/components/expanded_view/call_participant.tsx
@@ -60,54 +112,54 @@ export default function CallParticipant({
<>
<div style={{position: 'relative'}}>
<Avatar
size={50}
fontSize={18}
size={tileSizePropsMap[size].avatarSize}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Total nit, but what if you added a const prop = tileSizePropsMap[size]; at the start? Would clean up the code a bit.

@abhijit-singh
Copy link

@streamer45 The screen cap looks great. Could you please put this on a test server?

@streamer45
Copy link
Collaborator Author

@streamer45 The screen cap looks great. Could you please put this on a test server?

@abhijit-singh Done.

abhijit-singh
abhijit-singh requested changes Jul 5, 2024
View reviewed changes
Copy link

@abhijit-singh abhijit-singh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @streamer45! Looks like a huge improvement to me!

Just a couple of requests:

  1. If the username is displayed for a participant and it is long, it does not seem to be getting truncated properly. The full name does get truncated as expected even if it is long.
image
  1. Probably just a nice-to-have, but can we center align the new row of participants that's not fully populated. Here's screenshots to explain better -

Current: Left aligned bottom row (blank space on the right of participants)
image

Ideal: Center aligned bottom row (blank space on the left and right)
image

@streamer45
Copy link
Collaborator Author

@abhijit-singh I fear that neither is going to be straightforward:

  1. We are already using a non-stardard CSS feature to make it work properly on two lines. The easiest thing to do without adding more dynamic logic would be to simply break a very long string (e.g., the username you tested there) to the next line. Then, on two lines, the ellipsis would take care of it if it still exceeds the available space. If, instead, we want a per-line ellipsis, then it will be more tricky.
  2. That's how CSS grid is designed to work. To achieve centering of the last row, we'd have to go back to flex and see how it looks. I can check, but no promises.

@abhijit-singh
Copy link

The easiest thing to do without adding more dynamic logic would be to simply break a very long string

I'd be good with that. It is an edge case but how it renders currently clearly looks broken.

I can check, but no promises.

Not super confident if it adds value so I won't want you to spend too much time here. If it can be done without too much rework or breaking other things then that's awesome. Else we can choose to let it stay as is and review it again once we've used it for a bit.

@streamer45
Copy link
Collaborator Author

@abhijit-singh I haven't committed here yet but I updated the test server to see how it looks with flex. One side effect of that I notice is that now the grid items are not forced to be of equal height. It may also take a bit to ensure there's no funny breakage since we've been using grid for a long time. Let me know what you think, otherwise we can revert this bit.

abhijit-singh
abhijit-singh approved these changes Jul 9, 2024
View reviewed changes
Copy link

@abhijit-singh abhijit-singh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now the grid items are not forced to be of equal height.

Looks to me like they're still of equal heights when I tested so I'm not sure of the impact.

And seems perfect to me now. We can go ahead with this from a UX perspective.

Maybe we can take help from someone in QA to do comprehensive testing on this if we fear things might break?

@streamer45
Copy link
Collaborator Author

Looks to me like they're still of equal heights when I tested so I'm not sure of the impact.

@abhijit-singh It's more noticeable when you hover as host.

height.mp4

Maybe we can take help from someone in QA to do comprehensive testing on this if we fear things might break?

Yeah, we'll do some extra checking.

@abhijit-singh
Copy link

Aah got it. I think that's okay and expected.

I couldn't see it earlier since I had a host in both rows :P

image

@streamer45
Copy link
Collaborator Author

Aah got it. I think that's okay and expected.

I couldn't see it earlier since I had a host in both rows :P

image

Cool 👍

@streamer45 streamer45 added 2: QA Review Requires review by a QA tester and removed 1: UX Review Requires review by ux labels Jul 9, 2024
@streamer45 streamer45 removed the 2: Dev Review Requires review by a core committer label Jul 9, 2024
@streamer45 streamer45 requested a review from DHaussermann July 9, 2024 13:01
@streamer45
Copy link
Collaborator Author

@DHaussermann For this one, we'd like to check for any funny UX behaviour in the Calls pop-out based on the number of participants, their state, and different window sizes.

@streamer45 streamer45 added this to the v1.0.0 🚀 / MM 10.0 milestone Jul 12, 2024
DHaussermann
DHaussermann approved these changes Aug 13, 2024
View reviewed changes
Copy link

@DHaussermann DHaussermann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and passed.

  • Calls with several dozen participants uses horizontal space more efficiently
  • Participant profile are show in wider rows that span further towards the right and left edges as expected
  • Spot checked several odd and event participant counts raging form only a few up to over 60
  • As participant list grows the width of the rows grow gradually and the size of the the profile images gradually shrinks as expected
  • My test rig could not handle pushing the count up 75 - 100 but seems like the grid will scroll in the available horizontal resolution now rather than page.
  • Tested in browser and desktop as a precauton

LGTM!

@DHaussermann DHaussermann added 3: Reviews Complete All reviewers have approved the pull request and removed 2: QA Review Requires review by a QA tester labels Aug 13, 2024
@streamer45 streamer45 merged commit ce5ef91 into main Aug 14, 2024
19 checks passed
@streamer45 streamer45 deleted the MM-53118 branch August 14, 2024 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cpoile cpoile cpoile approved these changes

@abhijit-singh abhijit-singh abhijit-singh approved these changes

@DHaussermann DHaussermann DHaussermann approved these changes

Assignees

@streamer45 streamer45

Labels
3: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Milestone
v1.0.0 🚀 / MM 10.0
Development

Successfully merging this pull request may close these issues.
4 participants
@streamer45 @abhijit-singh @cpoile @DHaussermann