- Notifications
You must be signed in to change notification settings - Fork928
fix: display workspace avatars correctly when URLs fail to load#14814
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
cdr-botbot left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
This PR is no longer a hotfix.
- ✅ Base is main or release branch
- ❌ Has hotfix label
- ✅ Head is from coder/coder
- ✅ Less than 100 lines
matifali commentedSep 26, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@Parkreiner, a few changes in the UI tests are showing misalignment. |
Parkreiner commentedSep 26, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@matifali Yeah, sorry, I should've left a message. I was hoping this would be a hotfix, but after seeing how many stories would be changed, I decided that I'd put this through a full review process, and then went to bed Definitely not planning to push this through as-is, but appreciate you catching some wonkiness that crept through |
Parkreiner commentedSep 27, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Edit: never mind. Good to go
|
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.
Nothing about the logic here changed – just got formatted differently
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.
thank you 😄
| avatar = <Avatar src={src}>{title}</Avatar>; | ||
| avatar = ( | ||
| <Avatar background src={src}> | ||
| {typeof title === "string" ? title || "-" : imgFallbackText ?? "-"} |
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.
| {typeoftitle==="string" ?title||"-":imgFallbackText??"-"} | |
| {(typeoftitle==="string" ?title :imgFallbackText)||"-"} |
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.
Oh, good call
| /** | ||
| * Useful for when you need to pass in a ReactNode for the title of the | ||
| * component. | ||
| * | ||
| * MUI will try to take any string titles and turn them into the first | ||
| * character, but if you pass in a ReactNode, MUI can't do that, because it | ||
| * has no way to reliably grab the text content during renders. | ||
| * | ||
| * Tried writing some layout effect/JSX parsing logic to do the extraction, | ||
| * but it added complexity and render overhead, and wasn't reliable enough | ||
| */ |
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.
| /** | |
| *UsefulforwhenyouneedtopassinaReactNodeforthetitleofthe | |
| *component. | |
| * | |
| *MUIwilltrytotakeany stringtitlesandturnthemintothefirst | |
| *character,butifyoupassinaReactNode,MUIcan't do that, because it | |
| *hasnowaytoreliablygrabthetextcontentduringrenders. | |
| * | |
| *Triedwritingsomelayouteffect/JSXparsinglogictodotheextraction, | |
| *butitaddedcomplexityandrenderoverhead,andwasn't reliable enough | |
| */ | |
| /** | |
| *Usefulforwhenyouneedtopassina`ReactNode`forthetitleofthe | |
| *component,butyoustillwanttousethefirstcharacterofabitoftext | |
| *asthefallback. | |
| */ |
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.
thank you 😄
| } | ||
| return url.startsWith("/emojis/"); | ||
| } |
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.
hmm. I'm skeptic about adding special handling like this, but I get the motivation.
680e28b intomainUh oh!
There was an error while loading.Please reload this page.
No issue to link – this was an issue that surfaced for certain URL types when kicking the tires on the orgs work
Changes made
AvatarDatacomponent to let you manually specify the source of the text used when images fail to load, and updated the orgs breadcrumb segment to use it