[Programs] Fix LFX sidebar scroll - #7922
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe changes add a shared scroll offset constant in Programs.style.js, combining navbar height and section gap, and apply it as scroll-margin-top on anchored elements. LfxPageNav.js uses this constant for active-section detection and adds a click handler for smooth scrolling with history updates. ChangesLFX Navigation
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant LfxPageNav
participant ProgramsWrapper
participant BrowserHistory
User->>LfxPageNav: Click navigation link
LfxPageNav->>ProgramsWrapper: Smoothly scroll to target section
LfxPageNav->>BrowserHistory: pushState with target URL
LfxPageNav->>ProgramsWrapper: Use shared offset for active-section detection
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Signed-off-by: hiyach28 <hiyach28@gmail.com>
e916e9c to
4d11b1e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/components/Card/Card.style.js (1)
74-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse theme values for the new thumbnail dimensions.
Lines 78 and 136 add raw
2remand10remvalues. Define and use theme values for this padding and height.As per coding guidelines: “Use theme values for styled-components.”
Also applies to: 131-136
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/Card/Card.style.js` around lines 74 - 83, Replace the raw 2rem padding and 10rem height values in the Card styled-component rules with appropriate theme-based values. Define the required spacing and dimension tokens in the theme if they do not already exist, then reference them through the styled-components theme while preserving the existing $fitContainer and thumbnail layout behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/collections/programs/lfx-2026/LfxPageNav.js`:
- Around line 99-105: Update handleNavClick to avoid calling history.pushState
when href already matches the current URL hash, while preserving smooth
scrolling. Extract or reuse the existing scroll and activeHref update logic, and
add a popstate listener that applies it when browser Back/Forward changes the
hash; ensure the listener is registered and cleaned up with the component
lifecycle.
- Around line 99-105: Update handleNavClick to bypass custom interception for
modified clicks (Ctrl/Cmd, Shift, or Alt) and non-primary mouse buttons,
allowing native link activation; call preventDefault only for an unmodified
primary-button click before scrolling and updating history.
---
Nitpick comments:
In `@src/components/Card/Card.style.js`:
- Around line 74-83: Replace the raw 2rem padding and 10rem height values in the
Card styled-component rules with appropriate theme-based values. Define the
required spacing and dimension tokens in the theme if they do not already exist,
then reference them through the styled-components theme while preserving the
existing $fitContainer and thumbnail layout behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 152eb8de-2e6f-4b7a-ae9a-b99a88a16595
📒 Files selected for processing (6)
src/collections/programs/Programs.style.jssrc/collections/programs/lfx-2026/LfxPageNav.jssrc/components/Card/Card.style.jssrc/components/Card/index.jssrc/components/image.jssrc/sections/Resources/Resources-grid/index.js
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/collections/programs/lfx-2026/LfxPageNav.js (1)
15-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a theme value for the
NavCardshadow.This declaration hard-codes
rgba(0, 0, 0, 0.1)in a styled component. Use the existing theme shadow token, or add one to both theme variants. This keeps the card consistent across themes.As per coding guidelines: Use theme values for styled-components.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/collections/programs/lfx-2026/LfxPageNav.js` at line 15, Update the NavCard styled component’s box-shadow declaration to use the existing theme shadow token instead of the hard-coded rgba value; if no suitable token exists, add the shadow token consistently to both theme variants and reference it through the theme.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/collections/programs/lfx-2026/LfxPageNav.js`:
- Line 15: Update the NavCard styled component’s box-shadow declaration to use
the existing theme shadow token instead of the hard-coded rgba value; if no
suitable token exists, add the shadow token consistently to both theme variants
and reference it through the theme.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b06f5b1b-6c83-405a-ad1c-3c2467f8cc6f
📒 Files selected for processing (2)
src/collections/programs/Programs.style.jssrc/collections/programs/lfx-2026/LfxPageNav.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/collections/programs/Programs.style.js
rishiraj38
left a comment
rishiraj38
left a comment
There was a problem hiding this comment.
Please address the CodeRabbit review comments. Make sure to reply to every suggestion, even if you've already addressed it. If you didn't apply a suggestion, reply with a clear explanation of why it wasn't applied.
|
@rishiraj38 yes, I'm already working on it. |
fc5e488 to
0e644b1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/collections/programs/lfx-2026/LfxPageNav.js (1)
15-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a theme value for the card shadow.
This declaration still hardcodes
rgba(0, 0, 0, 0.1). Use an existing shadow token, or add one to the theme, soNavCardfollows the design system.As per coding guidelines, styled-components must use theme values.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/collections/programs/lfx-2026/LfxPageNav.js` at line 15, The box-shadow property in NavCard contains a hardcoded rgba color value instead of using a theme token. Replace the hardcoded box-shadow declaration with a reference to an existing shadow token from the theme, or add a new shadow token to the theme if one does not exist that matches the intended shadow effect. Reference the theme shadow token using the styled-components theme prop to ensure NavCard follows the design system guidelines.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/collections/programs/lfx-2026/lfx-2026.mdx`:
- Line 25: Update the lfx-layout markup and its associated layout direction so
DOM order matches the intended visual order on mobile: either render
lfx-sidebar-col before the main content and use the appropriate desktop row
direction to keep it on the right, or remove the mobile column-reverse behavior
in Programs.style.js and use column. Preserve semantic structure and WCAG 2.1 AA
keyboard and screen-reader navigation order.
---
Nitpick comments:
In `@src/collections/programs/lfx-2026/LfxPageNav.js`:
- Line 15: The box-shadow property in NavCard contains a hardcoded rgba color
value instead of using a theme token. Replace the hardcoded box-shadow
declaration with a reference to an existing shadow token from the theme, or add
a new shadow token to the theme if one does not exist that matches the intended
shadow effect. Reference the theme shadow token using the styled-components
theme prop to ensure NavCard follows the design system guidelines.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9afc33c3-28c5-4b09-81be-cb7c75f71833
📒 Files selected for processing (3)
src/collections/programs/Programs.style.jssrc/collections/programs/lfx-2026/LfxPageNav.jssrc/collections/programs/lfx-2026/lfx-2026.mdx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/collections/programs/Programs.style.js
33aa5db to
4d11b1e
Compare
|
Apologies for the confusion caused by the conversation history. The two commits referenced above were unrelated to this issue and were accidentally introduced while I was working on a different PR and failed to switch branches appropriately. This has since been corrected, and the branch now contains only the commits relevant to this issue. Sorry for the noise. |
…ate pushState - Guard handleNavClick so Ctrl/Cmd/Shift-clicks fall through to the browser native behavior (open in new tab / new window) instead of being swallowed by preventDefault. - Skip window.history.pushState when the hash is already the current URL hash, preventing duplicate Back/Forward history entries on repeated clicks to the same section. - Strip trailing whitespace from Programs.style.js. Signed-off-by: hiyach28 <hiyach28@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/collections/programs/lfx-2026/LfxPageNav.js (1)
80-97: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSync
activeHrefand scroll position on browser Back/Forward navigation.The
scrolllistener updatesactiveHrefand clicking a nav link pushes a new history entry viahandleNavClick, but there is nopopstatelistener. When a user navigates Back/Forward, the URL hash changes without triggering ascrollevent large enough to updateactiveHref, and the page does not scroll to the new hash target. This was raised in a previous review and remains unaddressed.Add a
popstatelistener that re-runs the scroll-to-target andactiveHrefupdate logic when the hash changes via browser history.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/collections/programs/lfx-2026/LfxPageNav.js` around lines 80 - 97, Add a popstate event listener within the same useEffect hook that runs after the scroll listener is attached. When the browser's Back/Forward button changes the URL hash, the popstate listener should scroll to the element matching the new hash and update activeHref using the same logic contained in the onScroll function. Ensure the popstate listener is removed in the cleanup function alongside the existing scroll listener removal, and verify that both listeners reference the same items dependency.
♻️ Duplicate comments (1)
src/collections/programs/lfx-2026/LfxPageNav.js (1)
99-101: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd the missing
altKeycheck to the modifier guard.
handleNavClickskips interception fore.ctrlKey,e.metaKey,e.shiftKey, and non-primary buttons, but note.altKey. A previous review requested a check that also coverse.altKey. In some browsers, Alt+click on a link triggers a "Save Link As" download instead of navigation; intercepting that click withpreventDefault()breaks that native behavior.Proposed fix
- if (e.ctrlKey || e.metaKey || e.shiftKey || e.button !== 0) return; + if (e.ctrlKey || e.metaKey || e.shiftKey || e.altKey || e.button !== 0) return;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/collections/programs/lfx-2026/LfxPageNav.js` around lines 99 - 101, Add the missing altKey check to the modifier guard in handleNavClick. Update the condition that checks for e.ctrlKey, e.metaKey, e.shiftKey, and e.button !== 0 to also include e.altKey so that Alt+click events are not intercepted, preserving the browser's native "Save Link As" behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/collections/programs/Programs.style.js`:
- Around line 8-11: The scroll-margin-top calculation in ProgramsWrapper
currently subtracts 5rem from LFX_SCROLL_OFFSET, resulting in only 20px offset,
while the active section detection in LfxPageNav.js uses the full
LFX_SCROLL_OFFSET value of 100px. Update the scroll-margin-top in
ProgramsWrapper to use the full LFX_SCROLL_OFFSET without the 5rem subtraction
to match the offset used in LfxPageNav.js, ensuring anchor clicks and active
section detection remain consistent.
---
Outside diff comments:
In `@src/collections/programs/lfx-2026/LfxPageNav.js`:
- Around line 80-97: Add a popstate event listener within the same useEffect
hook that runs after the scroll listener is attached. When the browser's
Back/Forward button changes the URL hash, the popstate listener should scroll to
the element matching the new hash and update activeHref using the same logic
contained in the onScroll function. Ensure the popstate listener is removed in
the cleanup function alongside the existing scroll listener removal, and verify
that both listeners reference the same items dependency.
---
Duplicate comments:
In `@src/collections/programs/lfx-2026/LfxPageNav.js`:
- Around line 99-101: Add the missing altKey check to the modifier guard in
handleNavClick. Update the condition that checks for e.ctrlKey, e.metaKey,
e.shiftKey, and e.button !== 0 to also include e.altKey so that Alt+click events
are not intercepted, preserving the browser's native "Save Link As" behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 704e50c9-7b32-4c92-82ef-28f7bc52cc90
📒 Files selected for processing (2)
src/collections/programs/Programs.style.jssrc/collections/programs/lfx-2026/LfxPageNav.js
|
I investigated this by prototyping a popstate-based implementation that reused the same section-scrolling logic as the sidebar navigation. After testing in the browser, the behavior remained unchanged and did not resolve the history navigation positioning issue. Addressing this appears to require more invasive browser history or scroll-restoration handling (for example, manipulating history restoration behavior or introducing timing-based workarounds), which is beyond the scope of this PR. Since the primary sidebar navigation behavior is working correctly, I've kept this PR focused and avoided introducing additional complexity for browser history traversal. The Back/Forward behavior can be revisited separately if we decide to support custom history restoration in the future. |
Add altKey modifier guard to preserve native browser behaviour for Save Link As. Guard pushState to avoid duplicate history entries. Signed-off-by: hiyach28 <hiyach28@gmail.com>
|
🚀 Preview deployment: https://layer5.io/pr-preview/pr-7922/ |
3 participants
Description
This PR fixes #7920
Previously, navigating through the sidebar produced inconsistent scroll behavior:
This PR makes anchor navigation deterministic while ensuring section headings align consistently beneath the fixed navigation.
Root Cause
The issue was caused by two separate concerns:
Anchor positioning
Native anchor navigation needed an explicit offset to account for the fixed navigation bar.
Layout spacing
The original visual gap above sections was larger than necessary, causing the end of the previous section to remain visible after navigation.
Solution
This implementation introduces a localized scroll offset for the LFX Programs page.
CSS-first approach:
The solution relies on native browser scrolling using
scroll-margin-top, allowing the browser to correctly position anchor targets beneath the fixed navigation bar.The offset is applied only to anchor targets within the Programs page, avoiding changes to global scrolling behavior.
Shared offset:
The sidebar navigation and anchor positioning now use the same logical scroll offset, ensuring:
- consistent landing positions
- consistent active-section highlighting
- no duplicated magic numbers
This solution was drafted keeping in mind: future changes of alignment or positioning of text will not break the scroll behaviour, and retain the correct scroll behaviour and position.
Testing
Verified locally:
Screenshots
Before
Screen.Recording.2026-08-04.150755.mov
After
Screen.Recording.2026-08-04.170607.mov
Signed commits
Summary by CodeRabbit
Bug Fixes
Enhancements