Skip to content

refactor: replace scroll-sentinel id with lvt-scroll-sentinel attribute#72

Open
adnaan wants to merge 1 commit intomainfrom
lvt-scroll-sentinel
Open

refactor: replace scroll-sentinel id with lvt-scroll-sentinel attribute#72
adnaan wants to merge 1 commit intomainfrom
lvt-scroll-sentinel

Conversation

@adnaan
Copy link
Copy Markdown
Contributor

@adnaan adnaan commented Apr 20, 2026

Summary

  • Updates infinite scroll template to use lvt-scroll-sentinel attribute instead of id="scroll-sentinel"
  • Updates E2E test JS selectors from getElementById to querySelector
  • Updates handler comment

Depends on: livetemplate/client#92 (must be merged + published first)

Test plan

  • TestInfiniteScroll E2E passes with updated client build (all 3 subtests)
  • Scroll_Triggers_More_Pages verified with browser-based IntersectionObserver

🤖 Generated with Claude Code

…bute

Updates the infinite scroll template and E2E tests to use the new
lvt-scroll-sentinel boolean attribute instead of the old plain HTML
id, aligning with the framework's lvt-* attribute convention.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings April 20, 2026 01:39
@claude
Copy link
Copy Markdown

claude bot commented Apr 20, 2026

Code Review

Overview

This PR refactors the infinite scroll sentinel from an id-based selector (id="scroll-sentinel") to a framework-native lvt-* attribute (lvt-scroll-sentinel). The change is small, consistent across all three affected files, and aligns with LiveTemplate's Tier 2 attribute conventions.


What's Good

  • Correct idiom. Using lvt-scroll-sentinel instead of an id is the right move — framework behavior should be signaled by lvt-* attributes, not by ad hoc IDs. This is consistent with how other Tier 2 constructs are named.
  • Selector update is correct. document.querySelector('[lvt-scroll-sentinel]') is the proper way to select a boolean attribute; switching from getElementById is correct and handles zero-or-one matches the same way.
  • All three surfaces updated together. Template, handler comment, and test comment/selector are all in sync — no stale references left behind in the diff.

Issues / Concerns

1. Hard dependency on an unmerged client release (blocking)

The PR description says:

Depends on: livetemplate/client#92 (must be merged + published first)

If this is merged before the client library is updated and published to the CDN, every user hitting the production CDN build will get a broken infinite scroll (the IntersectionObserver won't find the sentinel). Consider:

  • Adding a CI guard (e.g., a go.mod/package.json version pin check) that fails if the required client version isn't present.
  • Or: keep this branch off main until client#92 ships, and note that in the PR title/labels.

2. Stale id may exist elsewhere

The diff only touches three files. If any CSS, documentation, or other templates reference #scroll-sentinel, those will silently break. Worth a quick repo-wide search:

grep -r "scroll-sentinel" .

(If the result is clean, add a note to the PR confirming it.)

3. Boolean attribute in the template

<div lvt-scroll-sentinel>

This is valid HTML5, but some HTML validators and linters flag valueless custom attributes. If the project runs an HTML validator in CI, verify it's happy with this form. An explicit empty-string value (lvt-scroll-sentinel="") is equally valid and sometimes more tool-friendly, though either works with querySelector('[lvt-scroll-sentinel]').


Minor Nit

The description <p><small>A single <code>&lt;div lvt-scroll-sentinel&gt;</code> at the end…</small></p> renders the attribute correctly — no issues there.


Summary

The refactor is clean and directionally correct. The only real risk is the client dependency ordering. Resolve that (merge gate or version pin) and confirm no other scroll-sentinel references exist, and this is good to merge.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Refactors the infinite-scroll “sentinel” hook from a hard-coded id="scroll-sentinel" to a lvt-scroll-sentinel attribute, aligning templates and E2E tests with the updated client behavior.

Changes:

  • Updated infinite scroll template to use <div lvt-scroll-sentinel> (and updated explanatory text).
  • Updated E2E test selectors from getElementById to querySelector('[lvt-scroll-sentinel]').
  • Updated handler comment to reflect the new sentinel markup.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
patterns/templates/lists/infinite-scroll.tmpl Replaces sentinel id usage with lvt-scroll-sentinel attribute in markup and docs text.
patterns/patterns_test.go Updates chromedp JS to locate the sentinel via attribute selector.
patterns/handlers_lists.go Updates comment to match the new sentinel attribute.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

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.

2 participants