Sid/git repo header #195
Sid/git repo header #195
Conversation
| setIsOpen(!isOpen); | ||
| window.open('https://github.com/VikeLabs/courseup'); | ||
| }; | ||
| return <IconButton aria-label="GitHub" icon={<DiGithubBadge />} borderRadius="50%" name="github" onClick={open} />; |
There was a problem hiding this comment.
Instead of hooking into the state, you can add the props as="a" href="https://github.com/VikeLabs/courseup" which will do the same thing. Additionally, if you do it this way, you can add target="_blank" so clicking the link opens a new tab.
There was a problem hiding this comment.
nit: you can use the isRound prop instead of doing borderRadius="50%" (chakra docs for reference)
There was a problem hiding this comment.
my fault for not creating a mock-up to base styling off of, but if we can aim to make it look like the attached screenshot that would be 🔥
There was a problem hiding this comment.
The black colour scheme would be better for a11y.
https://contrastchecker.online/000000/82cbee whereas the white variant wouldn't pass.
Another approach would be to change the header colour outright and fix some more issues.
If we want to use the white variant here we should try and make the title (CourseUp) and this GitHub logo compliant in the near future.
There was a problem hiding this comment.
Thanks for the feedback guys! I'll change the code accordingly and request another review.
| <TermButtons /> | ||
| </GridItem> | ||
| <GridItem colStart={4}> | ||
| <ExternalLinks /> |
There was a problem hiding this comment.
If you opt for going with the props over the useState approach, this doesn't have to be it's own component
There was a problem hiding this comment.
We shouldn't consider this approach as there isn't a need for useState and complicates the code unnecessarily let alone accessibility issues I imagine.
|
@siddharthpk Hey! Just checking in to see the status of this PR. Are you still working on this? (no rush, just a check-in) |
|
Oh yeah, last and this week has been midterms so gonna complete this during the weekend. |
|
Closing due to inactivity. More than welcome to open a new PR if the issue is still open. |
3 participants
Overview
Closes #163
Added a redirect to the Github repo using an icon button.
Before
After