feat: new active rounds section in secondary navbar#2346
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
🥷 Code experts: bhargavaparoksham, thelostone-mc vacekj, bhargavaparoksham have most 👩💻 activity in the files. See details
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame: To learn more about /:\ gitStream - Visit our Docs |
|
This PR is 43.43434343434344% new code. |
boudra
left a comment
boudra
left a comment
There was a problem hiding this comment.
remember to use git mv to rename or move files, it's really difficult to see the changes
|
@vacekj just want to call out that the button's background should be white and not gray, otherwise lgtm! |
|
+1 to Melissa's comment - otherwise lgtm as well! |
|
@melissa-neira should the bg be white only when the dropdown is active? otherwise, according to figma, the bg of the button is |
|
@vacekj the bg should always be white, my bad - updated the figma |
1e54438
|
@melissa-neira fixed |
…showcase-upcoming-rounds
| ]; | ||
|
|
||
| export function RoundsSubNav(props: Props) { | ||
| const [open, setOpen] = useState(false); |
There was a problem hiding this comment.
Personally I like to name this isOpen so it doesn't take reading any extra code to know that it's a boolean value rather than an action.
| data-testid={`round-link-${round.id}`} | ||
| href="#" | ||
| onClick={(e: React.MouseEvent<HTMLAnchorElement>) => { | ||
| e.preventDefault(); |
There was a problem hiding this comment.
What happens if this is not called?
Edit: nevermind, I see now that this is inside an <a/>. Can the <a/> maybe be dropped?
There was a problem hiding this comment.
Not the author of the choice, but I presume it's to preserve the a11y semanatics of this being a link? We can make it a button, but it isn't really a button - it's a link. I would be more for using React Router's Component to both preserve the original semantics and to make it work with react router.
| case ChainId.FANTOM_MAINNET_CHAIN_ID: | ||
| return "/logos/fantom-logo.svg"; | ||
| } | ||
| })()} |
There was a problem hiding this comment.
If you use the ternary here, you avoid the odd-looking inline call and force yourself to make all outcomes explicit:
src={
round.chainId === ChainId.ARBITRUM
? "/logos/arb-logo.svg"
: round.chainId === ChainId.FANTOM_MAINNET_CHAIN_ID
? "/logos/fantom-logo.svg"
: null;
}There was a problem hiding this comment.
True, I was indulging in a poor-man's Rust-like match expression, but agree that it's not that readable and a ternary is better. (wen https://github.com/tc39/proposal-pattern-matching)
6 participants
fixes #2275