[Site] Increase depth of in-page table of contents on LFX programs page - #7901
[Site] Increase depth of in-page table of contents on LFX programs page#7901Sathwik-parimi-07 wants to merge 4 commits into
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:
📝 WalkthroughWalkthroughLFX 2026 project subsections now expose explicit anchors. ChangesLFX nested project navigation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant LfxPageNav
participant Lfx2026Document
Browser->>LfxPageNav: Scroll event
LfxPageNav->>Lfx2026Document: Check section offsets
Lfx2026Document-->>LfxPageNav: Active section position
LfxPageNav-->>Browser: Update active parent and child links
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: dependency version conflict. Check your lock file or package.json. 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 |
|
Hii @KhushamBansal, Could you please review it? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 101-102: Update the effect that registers the scroll handler in
LfxPageNav so it invokes the handler once immediately after registration,
initializing the active navigation state on mount while preserving subsequent
scroll updates.
🪄 Autofix (Beta)
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: e73f546d-f9f2-4372-997d-ce849b952cd9
📒 Files selected for processing (2)
src/collections/programs/lfx-2026/LfxPageNav.jssrc/collections/programs/lfx-2026/lfx-2026.mdx
KhushamBansal
left a comment
KhushamBansal
left a comment
There was a problem hiding this comment.
@Sathwik-parimi-07 Could you please add before and after screenshots to the PR description and also address coderabbit's feedback?
| const [showTop, setShowTop] = useState(false); | ||
| const [activeHref, setActiveHref] = useState(""); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
Hi @Sathwik-parimi-07, thanks for the PR! 🙌
One small fix: window.addEventListener("scroll", onScroll) is currently inside the flatItems.forEach loop, so it registers multiple listeners and causes scroll lag. Please move it outside the loop and call onScroll() once on mount to set the initial state.
There was a problem hiding this comment.
Thanks @AnkitRewar11, one small clarification that window.addEventListener("scroll", onScroll) is already written outside the loop items.forEach I think the hunk boundary makes it look nested. your point below about the initial state is spot on, working on it. once ill check everything on local and commit again
| setActiveHref(current); | ||
| }; | ||
| window.addEventListener("scroll", onScroll); | ||
| return () => window.removeEventListener("scroll", onScroll); |
There was a problem hiding this comment.
one more small improvement. The active sidebar link is not highlighted until the user scrolls when the page with a hash URL is opened or the page is refreshed. After the scroll listener is registered, call onScroll() once so that the correct link is highlighted when the page loads.
|
Also, please add before & after screenshots to the PR description, as @KhushamBansal requested. |
|
Okk thanks @AnkitRewar11, I'm working on it. will update the PR soon |
Signed-off-by: Sathwik_parimi <sathwikparimi777@gmail.com>
Signed-off-by: Sathwik_parimi <sathwikparimi777@gmail.com>
4357e3a to
ddd459a
Compare
There was a problem hiding this comment.
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)
125-144: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winExpose the active section to assistive technologies.
The current location is communicated only visually through
className="active". Addaria-current="location"to the active parent and child links.Suggested fix
<a href={item.href} className={activeHref === item.href ? "active" : ""} + aria-current={activeHref === item.href ? "location" : undefined} > ... <a href={child.href} className={activeHref === child.href ? "active" : ""} + aria-current={activeHref === child.href ? "location" : undefined} >As per coding guidelines, JSX changes must meet WCAG 2.1 Level AA accessibility requirements.
🤖 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 125 - 144, Add aria-current="location" to the parent and child link elements in LfxPageNav, setting it only when each link matches activeHref and leaving it unset for inactive links. Preserve the existing active class behavior.Source: Coding guidelines
🧹 Nitpick comments (1)
src/collections/programs/lfx-2026/LfxPageNav.js (1)
101-105: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winThrottle the scroll-driven navigation scan.
This now rebuilds and scans every parent and child item synchronously for every scroll event. Use a passive listener with
requestAnimationFrameor throttling, and cancel any pending frame during cleanup.As per coding guidelines, expensive event handlers must be debounced or throttled.
🤖 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 101 - 105, Throttle the scroll handler around the flatItems scan so it does not rebuild and iterate synchronously on every scroll event. Register the listener as passive, schedule the navigation work through requestAnimationFrame (or the existing throttle utility), and cancel any pending frame during the effect/component cleanup while preserving the current scan 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.
Outside diff comments:
In `@src/collections/programs/lfx-2026/LfxPageNav.js`:
- Around line 125-144: Add aria-current="location" to the parent and child link
elements in LfxPageNav, setting it only when each link matches activeHref and
leaving it unset for inactive links. Preserve the existing active class
behavior.
---
Nitpick comments:
In `@src/collections/programs/lfx-2026/LfxPageNav.js`:
- Around line 101-105: Throttle the scroll handler around the flatItems scan so
it does not rebuild and iterate synchronously on every scroll event. Register
the listener as passive, schedule the navigation work through
requestAnimationFrame (or the existing throttle utility), and cancel any pending
frame during the effect/component cleanup while preserving the current scan
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d4155128-0ce0-45eb-93ec-9801eda6078a
📒 Files selected for processing (1)
src/collections/programs/lfx-2026/LfxPageNav.js
|
Before this PR: 
After this PR: 
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 125-140: Update the parent and child anchors rendered by
LfxPageNav to set aria-current="location" when activeHref matches the anchor’s
href, and omit the attribute otherwise. Keep the existing active class behavior
unchanged for both navigation levels.
🪄 Autofix (Beta)
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: be85f860-ee9b-4f3c-aff5-9bb3b69e7672
📒 Files selected for processing (2)
src/collections/programs/lfx-2026/LfxPageNav.jssrc/collections/programs/lfx-2026/lfx-2026.mdx
Signed-off-by: Sathwik_parimi <sathwikparimi777@gmail.com>
|
Hii @KhushamBansal, @AnkitRewar11 I've addressed coderabbit's review and Added Before and After screenshots. Please review it |
There was a problem hiding this comment.
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/lfx-2026.mdx (1)
281-281: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winTighten the documentation wording.
Use “to generate” at Line 281 and “cloud-native infrastructure” at Line 307.
Proposed wording fix
-... needs in order to generate a design Meshery will accept, ... +... needs to generate a design Meshery will accept, ... -Meshery manages cloud and cloud native infrastructure through Designs, ... +Meshery manages cloud and cloud-native infrastructure through Designs, ...As per coding guidelines, MDX content must use clear, professional American English.
Also applies to: 307-307
🤖 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/lfx-2026.mdx` at line 281, Update the documentation wording at the referenced passages: use “to generate” in the sentence describing the knowledge needed by a language model, and replace the corresponding wording at the later passage with “cloud-native infrastructure.” Preserve the surrounding meaning and use clear professional American English.Sources: Coding guidelines, Linters/SAST tools
🤖 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.
Outside diff comments:
In `@src/collections/programs/lfx-2026/lfx-2026.mdx`:
- Line 281: Update the documentation wording at the referenced passages: use “to
generate” in the sentence describing the knowledge needed by a language model,
and replace the corresponding wording at the later passage with “cloud-native
infrastructure.” Preserve the surrounding meaning and use clear professional
American English.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ba8687bc-405a-4324-bc0a-2ccbf7061ca2
📒 Files selected for processing (1)
src/collections/programs/lfx-2026/lfx-2026.mdx
| `; | ||
|
|
||
| const SubNavList = styled.ul` | ||
| list-style: none !important; |
There was a problem hiding this comment.
Could we avoid using !important here? It makes styles harder to override and maintain.
| padding-left: 0.85rem; | ||
| border-left: 1px solid ${(props) => props.theme.grey1D1817ToGreyE6E6E6}; | ||
| li { | ||
| list-style: none !important; |
3 participants
Description
This PR increases the depth of the "On this page" navigation card on the LFX programs page,
Notes for Reviewers
Signed commits
Summary by CodeRabbit
New Features
Improvements
aria-currentstate.Documentation