-
-
Notifications
You must be signed in to change notification settings - Fork 8.5k
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
fix(theme): mobile navbar & skipToContent should cover announcementBar #8163
Conversation
✅ [V2]
To edit notification comments on pull requests, go to your Netlify site settings. |
⚡️ Lighthouse report for the deploy preview of this PR
|
@slorber Please review this PR and let me know if you want any changes. |
Thanks Unfortunately it's not a good fix because it defeats the purpose of a change I did to make it possible to add a shadow to the announcement banner here: #7940 With this fix, the shadow is always covered by the navbar. Will look for a better solution and edit a bit this PR Also need to consider this: #8173 |
Thanks for giving it a try. Unfortunately after studying the problem I come to the conclusion that some Infima refactor is required to achieve what I want (facebookincubator/infima#275) and the best solution is to temporarily revert my changes made here: #7940 Will do that in this PR so that you'll be credited for your attempt as well 👍 |
Thank you for the review. |
… navbar." This reverts commit e7aaebf.
#8163) Co-authored-by: sebastienlorber <[email protected]>
Pre-flight checklist
Motivation
In mobile view the announcement banner covered the navbar. It has been fixed and the navbar is stacked on top so that the banner doesnt cover it anymore.Test Plan
Test links
Deploy preview: https://deploy-preview-8163--docusaurus-2.netlify.app/
Related issues/PRs
Closes #8141