[DEV-026] Navbar#45
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/white-van/dialog/504jo3hi6 |
jcserv
left a comment
jcserv
left a comment
There was a problem hiding this comment.
Great work! Few improvements:
- You'll need to make sure it passes the linter. Run
make prettierin the root directory and then solve any linting issues that pop up. - Could you make the Dialog text link to the homepage?
- Add on checks similar to the existing one to verify that the login button is there, that the logo is there, and that clicking on the logo goes to the homepage.
AipioxTechson
left a comment
AipioxTechson
left a comment
There was a problem hiding this comment.
Awesome Work!! Added some comments on some bookkeeping things!
| const boxBg = useColorModeValue("#343A40", "#343A40"); | ||
| const buttonBg = useColorModeValue("#2D9CDB", "#2D9CDB"); | ||
| const iconColor = useColorModeValue("#ECC94B", "light"); | ||
|
|
There was a problem hiding this comment.
Nice fix for this! we will probably need to implement a custom colorScheme so we can use the LightMode DarkMode components properly, but that can be done on a later date
| <Button m="1" size="sm"> | ||
| <Button m="1" size="sm" bg={buttonBg} color={textColor}> | ||
| Login | ||
| </Button> |
There was a problem hiding this comment.
Can we i18n Sign up and Login?
| return ( | ||
| <Flex w="100%" m="2" mb="4"> | ||
| <Box m="2"> | ||
| <Flex w="101%" mb="4" bg={boxBg} boxShadow="lg"> |
There was a problem hiding this comment.
Suggestion: Since this component is used outside of the main landmark, we need to provide a semantic landmark HTML element as the wrapper rather than it being a div, can we convert this to use a nav?
| <Flex w="101%" mb="4" bg={boxBg} boxShadow="lg"> | |
| <Flex w="101%" mb="4" bg={boxBg} boxShadow="lg" as="nav"> |
For reference: https://www.w3.org/TR/wai-aria-practices/examples/landmarks/HTML5.html
4 participants
[DEV-026] Navbar
Fixes: #26
Changes
Fixed the colours and margins of Navbar to match the figma stories.
Light mode and dark mode colours currently look the same, other than for the colour of the toggle icon.
Testing