-
Notifications
You must be signed in to change notification settings - Fork 10.7k
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
regression: UserAvatar
should always render BaseAvatar
#34061
base: release-7.1.0
Are you sure you want to change the base?
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 |
|
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## release-7.1.0 #34061 +/- ##
==============================================
Coverage 75.75% 75.75%
==============================================
Files 510 510
Lines 22078 22078
Branches 5387 5387
==============================================
Hits 16726 16726
Misses 4706 4706
Partials 646 646
Flags with carried forward coverage won't be shown. Click here to find out more. |
62944f2
to
6f54cf1
Compare
6f54cf1
to
41445d2
Compare
@@ -6,7 +6,7 @@ export type AvatarUrlContextValue = { | |||
getUserPathAvatar: { | |||
(username: string, etag?: string): string; | |||
(params: { userId: string; etag?: string }): string; | |||
(params: { username: string; etag?: string }): string; | |||
(params: { username?: string; etag?: string }): string; |
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.
how is this possible? a function that accepts an empty object will return something?
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.
@ggazzo did you checked the function?
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.
sure and still makes no sense rendering something if you have no info about thats why this params are wrong
you are trying to fix a different problem with a "naive" solution
your problem is the /invite @something
breaking right ?
for a small fraction the autocomplete uses one render type for a different item list thats why username is undefined there. but this is the real problema.
even this change solves the issue, I think you are covering the wrong place
Proposed changes (including videos or screenshots)
Introduced here: #32906
Previously we didn't check
userId
orusername
to renderBaseAvatar
, neither throw an Error. There are some cases thatusername
can beundefined
such asComposerBoxPopupUser
which will break the entire UI if we keep the current solution. The PR aims to always render theBaseAvatar
as we were doing before, because this component has an error validation when trying to load an invalid URL, rendering aSkeleton
as a fallbackIssue(s)
Steps to test or reproduce
Further comments
CORE-834