-
Notifications
You must be signed in to change notification settings - Fork 16
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
added the menu bar component #31
added the menu bar component #31
Conversation
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.
Looking good so far! Main thing I think needs to be done before merge is updating values for colours, typography, and spacing to be retrieved from the active theme rather than hard-coded.
Ideally we should be rendering React Router links instead of plain HTML links, but that can be done in a follow-up task next trimester.
There is also room for improvement in the HTML structure (I noted about nav
but also just noticed there's no ul
or li
s, and the hamburger icon should have an accessible label) but these things would make a good first task for someone next trimester if you don't have time to refine these things this tri. :)
display: flex; | ||
justify-content: space-between; | ||
align-items: center; | ||
background-color: #e97462; /* Primary Color */ |
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.
Please use the theme colours throughout. Syntax example:
background-color: ${props => props.theme.colors.primary};
import styled from 'styled-components'; | ||
|
||
// Menu Bar container styling | ||
export const StyledMenuBar = styled.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.
Non-blocking thought: Maybe this could be a HTML nav
element rather than a div
?
|
||
// MenuItem styling with updated secondary color for text | ||
export const MenuItem = styled.a` | ||
font-family: 'Inter Tight', sans-serif; /* Font Family */ |
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.
Please use theme values for font family, font size, font weight, padding, and margins throughout. You can find all the available theme values here in the default theme definition.
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.
All requested changes have been completed
Summary
What is the nature of this pull request?
Description of change
Implemented a responsive Menu Bar Component for Redback UI, featuring navigation links, a dropdown menu, and a hamburger menu for mobile devices. The component follows accessibility guidelines, including keyboard navigation. Styled Components were used for styling, and unit tests have been added to ensure functionality.
Planner card link
https://teams.microsoft.com/l/entity/com.microsoft.teamspace.tab.planner/tt.c_19:[email protected]_p_TOGdCftBI0mZ937fEIW0jsgAHPsL_h_1710632381880?tenantId=d02378ec-1688-46d5-8540-1c28b5f470f6&webUrl=https%3A%2F%2Ftasks.teams.microsoft.com%2Fteamsui%2FpersonalApp%2Falltasklists&context=%7B%22subEntityId%22%3A%22%2Fv1%2Fplan%2FTOGdCftBI0mZ937fEIW0jsgAHPsL%2Ftask%2FcuDQV948JUSITMLQeoou48gAH9vc%22%2C%22channelId%22%3A%2219%3Ac1b94839a3db4c6e8bf052aeadb33d9b%40thread.tacv2%22%7D
Readiness