Skip to content

Layer5 Navbar is Fixed Now#6295

Merged
jamieplu merged 1 commit intolayer5io:masterfrom
Dungeon-Masterji:mybranch
Mar 12, 2025
Merged

Layer5 Navbar is Fixed Now#6295
jamieplu merged 1 commit intolayer5io:masterfrom
Dungeon-Masterji:mybranch

Conversation

@Dungeon-Masterji
Copy link
Copy Markdown
Contributor

Description

This PR fixes #6285

Notes for Reviewers

Signed commits

  • Yes, I signed my commits.

@l5io
Copy link
Copy Markdown
Member

l5io commented Mar 9, 2025

🚀 Preview for commit 759386a at: https://67cd9f02e67f894800ec9efb--layer5.netlify.app

@falgunmpatel
Copy link
Copy Markdown
Contributor

@Dungeon-Masterji
You may notice that, the content is getting hidden behind the navbar. Adding a margin might fix it, but we can't determine the exact value that would work best across different screen sizes and layouts.

A better solution is to use position: sticky;. This ensures that the element stays positioned correctly relative to the viewport without overlapping the navbar, while still allowing it to scroll naturally with the page when needed. This way, the content automatically adjusts and remains visible without requiring hardcoded margins.

Let me know if you need any further clarifications.

Thanks!

transition-duration: 0.8s;
transition-timing-function: cubic-bezier(0.2, 0.8, 0.2, 1);


Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noticed an extra blank line—keeping changes minimal helps reviewers focus on the key updates. Would you mind removing it? Appreciate your efforts! 😊

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing that out! I've made the change. Your attention to detail is super helpful! 👍


const NavigationWrap = styled.header`
width: 100vw;
position: fixed;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
position: fixed;
position: sticky;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion! I've replaced position: fixed with position: sticky as you recommended. Initially, I had tried using sticky earlier, but didn't notice any difference. However,navbar re-implementing it, I see that it's working as expected now. The element stays positioned correctly relative to the viewport, without overlapping the navbar, and scrolls naturally with the page. This solution is definitely more elegant and flexible than hardcoding margins. Appreciate your guidance!

@l5io
Copy link
Copy Markdown
Member

l5io commented Mar 9, 2025

🚀 Preview for commit d054390 at: https://67cde0a7f36b0428e8cc1d4c--layer5.netlify.app

@vishalvivekm
Copy link
Copy Markdown
Contributor

@Dungeon-Masterji
Thank you for your contribution!
Let's discuss this during the website call on Monday at 5:30 PM IST

adding it as an agenda item to the meeting minutes.

@falgunmpatel
Copy link
Copy Markdown
Contributor

falgunmpatel commented Mar 10, 2025

@vishalvivekm I think its working for all screen sizes.

But the other issue I noticed here, is that the layer5 website's overall layout is not responsive for smaller screen sizes (like tablets, or mobile). You may notice an empty section towards the right side of the footer, for example.

Screenshot 2025-03-10 at 6 55 19 PM

@falgunmpatel
Copy link
Copy Markdown
Contributor

@Dungeon-Masterji,
Great job implementing the suggested changes! I wanted to share a quick tip that might help with future contributions.

When making updates based on feedback, adding a new commit is totally fine during the review process. However, before merging, it's a good practice to squash fix-up commits so the commit history stays clean and meaningful.

You can do this using:
git rebase -i HEAD~[number_of_commits]

or, if pushing changes after squashing:
git push --force

(Just be cautious when using --force, especially in shared branches!)

@falgunmpatel
Copy link
Copy Markdown
Contributor

falgunmpatel commented Mar 10, 2025

@Dungeon-Masterji, if possible, please try to complete this today. Otherwise, I can handle it this time. You can then take the opportunity to learn more about rebase and use it in future contributions. 😊

@vishalvivekm
Copy link
Copy Markdown
Contributor

@vishalvivekm I think its working for all screen sizes.

But the other issue I noticed here, is that the layer5 website's overall layout is not responsive for smaller screen sizes (like tablets, or mobile). You may notice an empty section towards the right side of the footer, for example.

It might just be the reason for header to not be sticky in mobile view, do you mind taking a look at fixing it ? @falgunmpatel

@falgunmpatel
Copy link
Copy Markdown
Contributor

@vishalvivekm I think its working for all screen sizes.

But the other issue I noticed here, is that the layer5 website's overall layout is not responsive for smaller screen sizes (like tablets, or mobile). You may notice an empty section towards the right side of the footer, for example.

It might just be the reason for header to not be sticky in mobile view, do you mind taking a look at fixing it ? @falgunmpatel

It's an issue, even when the navbar is not sticky.

@vishalvivekm
Copy link
Copy Markdown
Contributor

@falgunmpatel I acknowledge the fact we have this issue, I'm asking if this is something you could help resolve?

@vishalvivekm
Copy link
Copy Markdown
Contributor

@falgunmpatel I acknowledge the fact we have this responsive issue, I'm asking if this is something you could help resolve? This might just fix the navbar after these two prs get merged.

@falgunmpatel
Copy link
Copy Markdown
Contributor

Thanks for clarification @vishalvivekm.

Yes, I will be working on it. But for this contribution from @Dungeon-Masterji, it can be merged, when the commits are squashed. I will create an issue for responsive layout.

Copy link
Copy Markdown
Member

@GARY121github GARY121github left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just squash your commits, and you're good to go! 🚀

Signed-off-by: Aditya Raj <adityaraj20112005@gmail.com>

Update and squashed navigation.style.js

Signed-off-by: Aditya Raj <adityaraj20112005@gmail.com>

Update navigation.style.js

Signed-off-by: Aditya Raj <adityaraj20112005@gmail.com>

Update navigation.style.js

Signed-off-by: Aditya Raj <adityaraj20112005@gmail.com>
@l5io
Copy link
Copy Markdown
Member

l5io commented Mar 10, 2025

🚀 Preview for commit c414fc9 at: https://67cf33ef3f2ebe00ab002a3e--layer5.netlify.app

Copy link
Copy Markdown
Member

@leecalcote leecalcote left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good!

@leecalcote
Copy link
Copy Markdown
Member

@vishalvivekm I think its working for all screen sizes.
But the other issue I noticed here, is that the layer5 website's overall layout is not responsive for smaller screen sizes (like tablets, or mobile). You may notice an empty section towards the right side of the footer, for example.

It might just be the reason for header to not be sticky in mobile view, do you mind taking a look at fixing it ? @falgunmpatel

Thank you for suggesting this, @vishalvivekm and for taking care of it, @falgunmpatel... in here, I expect - #6306

@jamieplu jamieplu merged commit 5757577 into layer5io:master Mar 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Layer5 Navbar is No Longer Sticky

7 participants