Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Set up Avatars for Workspace Chats #7852
Set up Avatars for Workspace Chats #7852
Changes from 17 commits
08ffa3d
86a1bbc
ef23069
a2dc66d
57f4e6c
be4c679
a61cafb
425e3b7
d5a963b
6dd30c9
efe6be5
0dab98c
7195cd2
3817498
39cc286
3763739
4ef49bc
6543229
0690dee
5cc113e
d83c3c2
463e928
5981680
0a0509a
533f851
25fd44d
3218eb3
6ad97fb
95aa9cc
9fe06b7
f8f6975
bedfc70
7652cf3
adc6ccc
1e54819
ec1dcec
b223cf2
4aff4d6
79a40ba
2f73f0d
5b8b050
704be62
0e981d8
00e207c
4290b91
0a84fae
7818250
9d39368
752edb5
63d87a5
6577c11
b434b58
e2bdea4
729301d
dd69004
00fb84e
4775ec2
ea668d8
a199aa6
12ac881
d537a28
ca1b024
d55c0f3
7aa05fa
4687612
8846c8d
a7e2a00
40325c7
9e69491
d47652b
bbff6ef
41b6c25
f11c423
e827030
b95477c
5bcca5f
5ac4713
2ac7de7
e8fa8f3
41115dd
34097c8
dc34a8b
e3990d8
5dc0073
652a7d7
a884bc0
b78492a
565cf0c
3f66b17
4b79410
45b90ae
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 know this wasn't added here, but anyone know what is up with the returning an array with an empty string? What is that supposed to mean and why should it be "non-empty"?
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 believe we return an empty string because we're using an Icon component instead of using a url for an Image when we're rendering the Avatars for rooms. This could be null, or empty or I guess we could pass the Icon component itself here, but the way we built it just has RoomAvatar.js just ignore whatevers stored here anyway. I think having this be empty string satisfies some propType checks.
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.
Ok thanks for the explanation. Maybe we can return an empty array or allow the icons to be
undefined
and not have to say anything about how the report icons will be used or why the array must be non empty. But don't think we have to block this PR on it though - just something I was curious about.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.
@marcochavezf Did our changes to
Avatar
change this?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.
Ah no, this
getReportIcons
is called before we save reports into Onyx and we can't set the corresponding Icon components here, as I suggested in this comment I think (in order to move/merge the logic ofgetAvatarSources
togetReportIcons
) we'll need to implement a different approach or maybe removegetReportIcons
fromPersonalDetails
:https://github.com/Expensify/App/blob/main/src/libs/actions/PersonalDetails.js#L192-L211
But I can address it in another ticket if it's ok because my gut tells me this can break something and we'll require more time to address it :D