fix: VideoConf message not considers display avatar pref #33141
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
🦋 Changeset detectedLatest commit: 6a27c57 The changes in this PR will be included in the next version bump. This PR includes changesets to release 37 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #33141 +/- ##
===========================================
- Coverage 59.17% 59.16% -0.01%
===========================================
Files 2822 2822
Lines 68124 68137 +13
Branches 15149 15153 +4
===========================================
+ Hits 40314 40316 +2
- Misses 24978 24988 +10
- Partials 2832 2833 +1
Flags with carried forward coverage won't be shown. Click here to find out more. |
dougfabris
force-pushed
the
fix/videoconf-display-avatars
branch
from
78fe4d9 to
3d5471f
Compare
dougfabris
force-pushed
the
fix/videoconf-display-avatars
branch
from
3d5471f to
7666449
Compare
|
| await setUserPreferences(api, { displayAvatars: true }); | ||
| }); | ||
|
|
||
| test('should not render avatars in video conference message block', async () => { |
There was a problem hiding this comment.
Does it make sense to have different combinations like: should render the avatars if the user preference is true, also, what should happen when the list contains user with and without avatars?
There was a problem hiding this comment.
I don't see why making a test to check the default assertion for this preference, we already have a test to check if the message block is there and the preference is true by default, so I added a test just to check if its ignoring the avatars properly when changing the preference. But if you think that makes sense adding an extra assertion for that, I can add it
what should happen when the list contains user with and without avatars
Basically what I'm testing, if the avatars are there or not
There was a problem hiding this comment.
There's never "users without avatar" afaik. If the user has no avatar it's the default one...
| <Box display='flex' alignItems='center' mi='neg-x2'> | ||
| {users.slice(0, MAX_USERS).map(({ name, username }, index) => ( | ||
| <Box mi={2} key={index}> | ||
| <Avatar size='x28' alt={username || ''} title={showRealName ? name : username} url={getUserAvatarPath(username as string)} /> |
There was a problem hiding this comment.
Name can be undefined here, so we have to fallback to username when that happens
There was a problem hiding this comment.
yep, let's replace it with the hook we're isolating in ui-client
| displayAvatars?: boolean; | ||
| iconTitle?: string; | ||
| showRealName?: ISetting['value']; |
There was a problem hiding this comment.
Why not get this info from the context inside the component, instead of passing as props?
| @@ -97,6 +101,23 @@ const VideoConferenceBlock = ({ | |||
| } | |||
| }; | |||
|
|
|||
| const getMessageFooterText = useCallback( | |||
There was a problem hiding this comment.
I don't think this has to be a useCallback, since it's just called below, it won't trigger rendering.
Maybe instead we could have a useMemo, or define this function outside the component?
|
|
||
| const joinedUsernames = [...data.users] | ||
| .splice(0, MAX_USERS) | ||
| .map(({ name, username }) => (showRealName ? name : username)) |
There was a problem hiding this comment.
yep, let's replace it with the hook we're isolating in ui-client
| data.users.length > MAX_USERS | ||
| ? t('__usernames__and__count__more_joined', { | ||
| usernames: joinedUsernames, | ||
| count: data.users.length - MAX_USERS, | ||
| }) | ||
| : t('__usernames__joined', { usernames: joinedUsernames }); |
There was a problem hiding this comment.
Shouldn't this respect the useRealName setting aswell?
There was a problem hiding this comment.
I didn't realize that joinedUsernames was already doing the check. Can you change the name of that var to joinedNamesOrUsernames so that it's clearer?
There was a problem hiding this comment.
Shouldn't this respect the
useRealNamesetting aswell?
yep, let's replace it with the hook we're isolating in ui-client
Proposed changes (including videos or screenshots)
Depends on: #35031
Issue(s)
Steps to test or reproduce
Further comments
CORE-656