-
Notifications
You must be signed in to change notification settings - Fork 4
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
Update the mobile header menu nav width for large mobile screen size #653
Conversation
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces a new constant Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
@jamiehenson I’m not sure if this matches the requirements in the docs. If not, could we pass mobile class names so I can apply these styles? I just add the classes directly for now in this PR |
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 at the designs for the meganav, I see the requirement here. Yeah I'm not sure about the docs situation either as there were no designs for the md
(i.e. tablet) range so we currently use the mobile behaviour for the tablet range as well, as you know. This didn't come up in the design feedback though, perhaps Jamie hadn't settled on the meganav designs at that time.
It's up to Design as to whether we want the mobile nav behaviour (i.e. what you're striving for here) to be consistent across both cases or not. If not, you're right in that we'll need a prop to differentiate.
In terms of what's here, the CSS approach isn't right - it doesn't match the behaviour in the Figma prototype. This follows on from recent conversations in that we shouldn't just use approximate percentages for widths. Looking at the designs, the mobile nav has a maximum width of 560px that naturally collapses when the viewport is narrower than 560, so this is a matter of right aligning and conditionally (or not conditionally, depending on what design say) setting a max width.
fd3ff5a
to
dbaf6ff
Compare
@coderabbitai review |
✅ Actions performedReview triggered.
|
Thanks @jamiehenson for the feedback! I’ve already applied your comment to match the Figma prototype. The mobile nav now has a max width of 560px and collapses naturally when the viewport is narrower. For the matter if it is applicable to docs as well, now waiting from Jamie W for his response. |
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.
Good thinking with the use of the MAX_MOBILE_MENU_WIDTH
constant, but we can do this more straightforwardly and without the use of JS.
- we don't need to worry about
left
, as the component is right aligned and so we're not interested in its left alignment - with
w-[calc(100%-24px)]
, we factor in the component margins into the width calculation. I don't think we need to constantise this as it's only used here, but take a look at some similar stuff I do in this helper file that's about setting heights as a sign of where to take this if we want to do more with calculating widths later - may as well keep that max width constant alive by replacing
max-w-[560px]
with an assignment tomaxWidth
in thestyle
prop. Can also make the constant 560 as with the above the margin no longer has to be factored into the value
you are amazing @jamiehenson ! I made it super complicated that will solve it |
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.
Nice - just a comment about how best to set the max width but good to go after that 👍
89a871b
to
2d23859
Compare
2d23859
to
622c39a
Compare
Jira Ticket Link / Motivation
WEB-3842 - Update the mobile header to have a different width for large mobile screen size as one of the design requirement for the Meganav mobile view design here
Summary of changes
This pull request includes a change to the
src/core/Header.tsx
file to improve the layout and responsiveness of the mobile menu. The most important change involves modifying the CSS class and inline styles to better handle the menu's width and maximum height.Changes to improve layout and responsiveness:
src/core/Header.tsx
: Updated themobile-menu
div's CSS class to set the width tocalc(100%-24px)
and added amaxWidth
inline style of560px
to ensure the menu does not exceed this width.How do you manually test this?
View the header and changed to tablet view and the width should not be in full width
Reviewer Tasks (optional)
Merge/Deploy Checklist
Frontend Checklist
Summary by CodeRabbit
Summary by CodeRabbit