refactor/fix: Move sidebar from article to base layout #6623
Conversation
* refactor/fix: Move navbar to base layout * feat: Provider and hook to hide sidebar in not-found and error pages * test: Sidebar provider and hook * refactor/fix: Article layout responsiveness * feat: Sticky navbar, metabar and sidebar Signed-off-by: abizek <[email protected]>
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Signed-off-by: abizek <[email protected]>
abizek
force-pushed
the
fix/sidebar-reload
branch
from
1b45ec8
to
b796376
Compare
yes, it allows the banner to be moved, but the idea of the |
github-actions
bot
removed
the
github_actions:pull-request
|
Lighthouse Results
|
Unit Test Coverage Report
Unit Test Report
|
|
@ovflowd Cool. Take care and get well soon ❤️ |
| import { availableLocales } from '@/next.locales.mjs'; | ||
|
|
||
| const WithNavBar: FC = () => { | ||
| const { navigationItems } = useSiteNavigation(); | ||
| const { resolvedTheme, setTheme } = useTheme(); | ||
| const { pathname } = useClientContext(); | ||
| const pathname = usePathname(); |
There was a problem hiding this comment.
Use the client context hook. If by any chance you're using the regular usePathname because of the value being different from the client context hook, then something is wrong.
There was a problem hiding this comment.
In general I understand the effort done here, and appreciate it. But unfortunately the way this PR changes the fundamental logic of certain layouts, makes this PR almost impossible to be approved.
I know I'm being too pushy here, but I'm sure there are alternative ways of recording the position of the sidebar navigation and/or keeping it stale.
One ingineous solution that does not require change in the structure of the layouts is:
- Update the links in the Sidebar components to also pass a
stateto the Next.js Router (pretty much it will update thehistorylocation with a state; The state value is an object, have a property to be Y coordinates. - This state contains what is the scroll position at the time of click
- When doing the navigation have an effect that immediately reads the history location state and scrolls to said position
It might create a flicker effect (depending how the effect is done, as it might not) and provides a reasonable solution to restore the right position at the moment of navigation.
|
I will give scrolling to the previous position a try 👍 |
Signed-off-by: abizek <[email protected]>
Signed-off-by: abizek <[email protected]>
Signed-off-by: abizek <[email protected]>
|
I'm closing this PR as I've found an extremely simple solution based on vercel/next.js#49279 |
I feel that in the end all these approaches are gimmicks. It definitely seems like a bug on Next.js side of things 😢 |
Description
Validation
before-responsiveness.mp4
after-responsiveness.mp4
Related Issues
Fixes #6409
Possible fix for #6292
Check List
npx turbo formatto ensure the code follows the style guide.npx turbo testto check if all tests are passing.npx turbo buildto check if the website builds without errors.