Skip to content

fix(JumpLinks): updated demo code for active item#12590

Open
thatblindgeye wants to merge 1 commit into
patternfly:mainfrom
thatblindgeye:iss12223
Open

fix(JumpLinks): updated demo code for active item#12590
thatblindgeye wants to merge 1 commit into
patternfly:mainfrom
thatblindgeye:iss12223

Conversation

@thatblindgeye

@thatblindgeye thatblindgeye commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

What: Closes #12223

Also added additional line of logic for an issue I was noticing for scrollspy demos (last jump link item not becoming "current" if that section didn't have enough content).

@edewit I noticed the Keycloak issue referenced in the above PF issue that y'all may have implemented a fix for the issue? FWIW i believe the original issue may have been because the scrollable page container was no longer accurate; we now have the .pf-v6-c-page__main selector as the scrollable container, not .pf-v6-c-page__main-container (which both one of our demos that were broken as well as Keycloak were originally using, at least based on Keycloak PR 47651).

Additional issues:

Summary by CodeRabbit

  • Bug Fixes
    • Fixed Jump Links scroll-spy behavior so the final link becomes active when scrolling to the bottom of a page.
    • Updated the scroll target used by the Jump Links example for more accurate active-link tracking.

@coderabbitai

coderabbitai Bot commented Jul 24, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

JumpLinks scrollspy now activates the last link when its scroll container reaches the bottom. The scrollspy demo also targets .pf-v6-c-page__main instead of .pf-v6-c-page__main-container.

Changes

JumpLinks scrollspy

Layer / File(s) Summary
Handle final scrollspy item
packages/react-core/src/components/JumpLinks/JumpLinks.tsx, packages/react-core/src/demos/examples/JumpLinks/JumpLinksScrollspy.tsx
scrollSpy detects when the scroll container is at its bottom and sets the active index to the last item; the demo updates its scrollable element selector.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title is concise and accurately highlights the JumpLinks active-item fix.
Linked Issues check ✅ Passed The changes address #12223 by restoring JumpLinks active-item tracking in the demo and scrollspy behavior.
Out of Scope Changes check ✅ Passed All changes are directly related to the JumpLinks active-item tracking fix and demo adjustment.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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 `@packages/react-core/src/components/JumpLinks/JumpLinks.tsx`:
- Around line 147-148: Update the bottom-detection logic in the JumpLinks scroll
handling so it requires the container to be scrollable before setting
isAtBottom, excluding cases where scrollHeight equals clientHeight. Apply the
same guard to the corresponding logic around the alternate referenced lines,
preserving the existing bottom threshold for genuinely scrollable containers.
🪄 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: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 988c5eab-531a-4730-822f-7eb8cbabb833

📥 Commits

Reviewing files that changed from the base of the PR and between 8fd7fdb and b68d2d9.

📒 Files selected for processing (2)
  • packages/react-core/src/components/JumpLinks/JumpLinks.tsx
  • packages/react-core/src/demos/examples/JumpLinks/JumpLinksScrollspy.tsx

Comment on lines +147 to +148
const isAtBottom =
Math.ceil(scrollableElement.scrollTop + scrollableElement.clientHeight) >= scrollableElement.scrollHeight;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Only treat the container as “at bottom” when it actually scrolls.

When scrollHeight === clientHeight, this condition is true at scrollTop === 0, so a short/non-scrollable page immediately marks the last JumpLink active instead of the first/default item.

Proposed fix
     const isAtBottom =
-      Math.ceil(scrollableElement.scrollTop + scrollableElement.clientHeight) >= scrollableElement.scrollHeight;
+      scrollableElement.scrollHeight > scrollableElement.clientHeight &&
+      Math.ceil(scrollableElement.scrollTop + scrollableElement.clientHeight) >= scrollableElement.scrollHeight;

Also applies to: 160-162

🤖 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 `@packages/react-core/src/components/JumpLinks/JumpLinks.tsx` around lines 147
- 148, Update the bottom-detection logic in the JumpLinks scroll handling so it
requires the container to be scrollable before setting isAtBottom, excluding
cases where scrollHeight equals clientHeight. Apply the same guard to the
corresponding logic around the alternate referenced lines, preserving the
existing bottom threshold for genuinely scrollable containers.

@rebeccaalpert rebeccaalpert left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Works great!

@edewit

edewit commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Thanks for the fix, this looks like a solid improvement for #12223 (especially the bottom-of-scroll active-item case and selector update). For Keycloak, we still can’t revert our temporary ScrollForm workaround yet as we’re currently on PF React 5.4.x, while this PR is on main (PF6).

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.

Bug - JumpLinks- Current no longer being tracked

3 participants