- Notifications
You must be signed in to change notification settings - Fork1.1k
feat: add support buttons#20339
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
Uh oh!
There was an error while loading.Please reload this page.
site/src/modules/dashboard/Navbar/UserDropdown/UserDropdownContent.tsx OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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.
Nothing major, but there are a few things I think could be tightened up
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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.
looks good to me but i'll let the more experienced frontend people look into it :-)
thanks for this!
| }, | ||
| { | ||
| Name:"Join the Coder Discord", | ||
| Target:"https://coder.com/chat?utm_source=coder&utm_medium=coder&utm_campaign=server-footer", |
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 guess we could make a new coder.com/chat link? or at least add a utm_source to the discord.gg url which i think the statistics do give us (unsure, need to check)
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.
Thanks for flagging it! We can update the link in a follow up since I don't know who owns these links.
Would you like to re-take a look at this PR or is it ready for approval & merge? |
@mtojek I'm starting a review right now |
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'm actually a little more awake this time around, so I'm noticing a few more things I think we can tighten up.
Those wouldn't be major enough to block an approval, but I did notice some layout problems in some of the stories. Once those are fixed up, we should basically be ready to go
| Iconstring`json:"icon" yaml:"icon" enums:"bug,chat,docs"` | ||
| Iconstring`json:"icon" yaml:"icon" enums:"bug,chat,docs,star"` | ||
| Locationstring`json:"location,omitempty" yaml:"location,omitempty" enums:"navbar,dropdown"` |
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.
Just asking because it sounds like I'll be helping out more withcoder/coder's backend soon: what uses theenums struct tags?
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.
mainly swagger generator + if we enable go validation, then the field will have restricted values too.
site/src/modules/dashboard/Navbar/UserDropdown/UserDropdownContent.tsx OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| constuniqueLinks=appearance.support_links | ||
| ?appearance.support_links.filter( | ||
| (link,index,self)=> | ||
| index===self.findIndex((l)=>l.name===link.name), | ||
| ) | ||
| :undefined; |
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'm wondering if it might be better to make thesupportLinks prop required forNavbarView, and then change the checks to see whether the array is empty. That way we don't have to deal with both the empty and undefined cases
And if we do that, we can simplifyuniqueLinks to:
constuniqueLinks=[...newSet(appearance.support_links)]
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.
To be honest, I don't know if I'veever seen the thirdself argument used in professional code before
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.
Set is equality by identity. they'll be separate objects with different identities. you'd still have duplicates in your list using that code.
this kind of uniqueness constraint really feels like something that should be enforced by the backend imo, but if we need to do it on the frontend then I would use aMap and usename as the key, then get the list back by doing[...supportLinksMap.values()]
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.
specifically, here's my suggestion:
| constuniqueLinks=appearance.support_links | |
| ?appearance.support_links.filter( | |
| (link,index,self)=> | |
| index===self.findIndex((l)=>l.name===link.name), | |
| ) | |
| :undefined; | |
| constuniqueLinks=newMap<string,LinkConfig>(); | |
| for(constlinkofappearance.support_links){ | |
| if(!uniqueLinks.has(link.name)){ | |
| uniqueLinks.set(link.name,link); | |
| } | |
| } | |
| // and later get the list by doing `[...uniqueLinks.values()]` |
tho I still think that data hygiene concerns like this belong more in the backend than that frontend
it's also worth noting that this "call a function if it exists, and give me undefined if not" pattern is common enough that it has it's own operator:?.
ParkreinerOct 20, 2025 • 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.
Oh right, duh. I forgot that these were objects. Another option would be to do this:
constuniqueLinks=[ ...newMap(appearance.supportLinks?.map((l)=>[l.name,l])).values()];
Though the nuances change a little bit. Kayla's suggestion always keeps the first entry that is encountered when you have a conflict, while mine keeps whichever one occurs later in the array
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 wouldn't touch the backend side now, but we can improve it in a follow-up. I updated the frontend side as you suggested. Please let me know your thoughts.
Uh oh!
There was an error while loading.Please reload this page.
| return( | ||
| <> | ||
| {supportLinks.map((link)=>( |
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 part works, but it feels a little clunky to me, because the boundaries aren't super clear. When you look at just this component, you can tell that you're rendering out multiple links, but you don't know how they'll be laid out, because you have to look at the parent component to see what kind of container it's using. It's also hard to reuse the link styling/logic because you can't use one by itself
I feel like a better approach would be to change this component intoSupportButton (singular), and then update the navbar to this:
<divclassName="hidden md:block">{supportLinks.filter(isNavbarLink).map((link)=>(<SupportButtonname={link.name}target={link.target}icon={link.icon}location={link.location}/>))}</div>
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 think keeping the container/layout logic closer together also makes another question more obvious: do we want to make this aul list?
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.
Refactored intoSupportButton components, but had to move<div className="hidden md:block"> to iterations, otherwise buttons were unaligned as you spotted on Chromatic. Let me know if there is a better solution, but definitely don't want to refactor the entire file to ul/li :)
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.
Sorry, Marcin! I had to leave early yesterday because I wasn't feeling well, and forgot to approve before I left
I'd still like to see if it makes sense to swap in aul list, but we can do that later
f2a4105 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Fixes:#16804
This PR modifies Navbar to include customSupport buttons. Enterprise users will be able to change their content.