Today view: auto-scroll during drag and scroll to first unchecked #191

Merged

ldraney merged 4 commits from feature/today-view-ux-improvements into main 2026-06-10 01:42:31 +00:00

Conversation 2 Commits 4 Files changed 2 +22 -3

ldraney commented 2026-06-09 17:06:54 +00:00

Owner

Copy link

Summary

• Enable SortableJS auto-scroll during drag so the page scrolls when dragging items near the viewport edge — desktop and mobile
• On page load, smoothly scroll to the first unchecked queue item so users skip past completed items

Changes

• app/javascript/controllers/sortable_controller.js: Added scroll, scrollSensitivity, scrollSpeed, bubbleScroll, forceFallback options to SortableJS config. Added scrollToFirstUnchecked() method called on controller connect — finds first .queue-item:not(.is-completed), skips if it's already the first item, scrolls with 300ms delay for smooth UX.

Test Plan

• Load today view with several completed items at top — page smooth-scrolls to first unchecked after 300ms
• Load today view with no completed items — no scroll occurs
• Load today view with all items completed — no scroll occurs
• Drag a queue item toward the bottom edge — page auto-scrolls down
• Drag toward the top edge — page auto-scrolls up
• Test drag auto-scroll on mobile (touch drag)
• Verify drag-to-reorder and position persistence still works
• No regressions in property picker or last-week section

Review Checklist

• Passed automated review-fix loop
• No secrets committed
• No unnecessary file changes
• Commit messages are descriptive
• Feature flag needed? No — UX behavior change, no new workflow or service integration

Related Notes

• Closes #189 — Auto-scroll when dragging items in today view
• Closes #190 — Today view auto-scrolls to first unchecked item on load
• landscaping-assistant — project

## Summary - Enable SortableJS auto-scroll during drag so the page scrolls when dragging items near the viewport edge — desktop and mobile - On page load, smoothly scroll to the first unchecked queue item so users skip past completed items ## Changes - `app/javascript/controllers/sortable_controller.js`: Added `scroll`, `scrollSensitivity`, `scrollSpeed`, `bubbleScroll`, `forceFallback` options to SortableJS config. Added `scrollToFirstUnchecked()` method called on controller connect — finds first `.queue-item:not(.is-completed)`, skips if it's already the first item, scrolls with 300ms delay for smooth UX. ## Test Plan - [ ] Load today view with several completed items at top — page smooth-scrolls to first unchecked after 300ms - [ ] Load today view with no completed items — no scroll occurs - [ ] Load today view with all items completed — no scroll occurs - [ ] Drag a queue item toward the bottom edge — page auto-scrolls down - [ ] Drag toward the top edge — page auto-scrolls up - [ ] Test drag auto-scroll on mobile (touch drag) - [ ] Verify drag-to-reorder and position persistence still works - [ ] No regressions in property picker or last-week section ## Review Checklist - [ ] Passed automated review-fix loop - [ ] No secrets committed - [ ] No unnecessary file changes - [ ] Commit messages are descriptive - [ ] Feature flag needed? No — UX behavior change, no new workflow or service integration ## Related Notes - Closes #189 — Auto-scroll when dragging items in today view - Closes #190 — Today view auto-scrolls to first unchecked item on load - `landscaping-assistant` — project

ldraney added 1 commit 2026-06-09 17:06:54 +00:00

Today view: auto-scroll during drag and scroll to first unchecked ... 625a82df53

Enable SortableJS scroll options (scroll, scrollSensitivity, bubbleScroll,
forceFallback) so the page scrolls when dragging items near the viewport
edge — works on both desktop and mobile touch.

On page load, smoothly scroll to the first unchecked queue item so the
user doesn't have to manually scroll past completed items.

Closes #189, closes #190

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

ldraney commented 2026-06-09 17:08:16 +00:00

Author

Owner

Copy link

PR #191 Review

DOMAIN REVIEW

Tech stack: Rails 7 / Hotwired Stimulus / SortableJS -- front-end JavaScript controller, no back-end changes.

SortableJS scroll options (lines 12-16 of diff)

The five new options (scroll, scrollSensitivity, scrollSpeed, bubbleScroll, forceFallback) are all valid SortableJS configuration properties. forceFallback: true is the right call for mobile touch support -- it bypasses the native HTML5 drag-and-drop API, which is unreliable on iOS/Android, and ensures the SortableJS custom drag implementation is used consistently across platforms. scrollSensitivity: 80 and scrollSpeed: 10 are reasonable defaults.

scrollToFirstUnchecked() method (lines 29-36 of diff)

The implementation is clean and defensive:

• Correctly returns early if no unchecked items exist (all completed).
• Correctly returns early if the first unchecked item is already the first child (nothing completed above it -- no scroll needed).
• Uses scrollIntoView({ behavior: "smooth", block: "start" }) which is well-supported.
• The 300ms setTimeout delay gives the DOM time to settle after Turbo renders.

The CSS selectors .queue-item and .is-completed are confirmed present in both _queue_item.html.erb (line 1: class="queue-item <%= 'is-completed' if item.completed? %>") and application.css (line 389+). Selector coupling is tight and correct.

One observation: html { scroll-behavior: smooth; } is already set in application.css (line 36), so the behavior: "smooth" in scrollIntoView is redundant but harmless -- it makes the intent explicit at the call site, which is fine.

Accessibility: No accessibility regressions. The change is purely behavioral (auto-scroll), not structural. Users with prefers-reduced-motion will still get the scroll behavior since neither the CSS scroll-behavior: smooth nor the JS scrollIntoView respects that media query by default. This is a nit, not a blocker, since this is a work-crew internal tool, but noted below.

BLOCKERS

None.

This is a 16-line front-end-only UX improvement to an existing Stimulus controller. It adds no new API surface, no new user input handling, no auth changes, and no security-sensitive paths. The "no test coverage" BLOCKER criterion does not apply here because:

• The changes are SortableJS configuration options (library-level, not testable in RSpec request specs).
• scrollToFirstUnchecked() is a DOM manipulation method that would require a JS test runner (which this project does not have -- all specs are RSpec request-level).
• The PR test plan correctly lists manual verification steps for all scenarios.

NITS

1. prefers-reduced-motion consideration: Users who set prefers-reduced-motion: reduce in their OS will still get smooth-scrolling. Consider wrapping the scrollIntoView behavior in a motion check:

   const motion = window.matchMedia("(prefers-reduced-motion: reduce)").matches
   firstUnchecked.scrollIntoView({ behavior: motion ? "auto" : "smooth", block: "start" })
   

   Non-blocking since this is an internal crew tool, but good practice for accessibility.

2. Magic number 300: The setTimeout delay of 300ms is a reasonable guess but is not tied to any specific animation or render event. If Turbo Drive timing changes, this could race. A comment like // Wait for Turbo to finish rendering would make the intent clear. Alternatively, listening for turbo:load would be more robust, though likely overkill for this use case.

SOP COMPLIANCE

• Branch naming: Branch is feature/today-view-ux-improvements -- does not follow the {issue-number}-{kebab-case-purpose} convention. Expected something like 189-today-view-auto-scroll or 189-190-today-view-ux. Non-blocking but noted.
• PR body follows template: Has Summary, Changes, Test Plan, Review Checklist, Related Notes sections.
• Related section references issues: References #189 and #190 with Closes syntax.
• Related references plan slug: No plan slug referenced. Task stated "No plan slug" so this is expected, but noted for traceability.
• No secrets committed: Clean -- single JS file, no credentials.
• No unnecessary file changes: Exactly one file changed, directly related to the issue scope.
• Commit messages: Not visible in the diff but PR title is descriptive.
• Tests: Manual test plan is appropriate for this type of change (front-end behavior in a project with no JS test runner).

PROCESS OBSERVATIONS

• Change failure risk: Low. This is additive configuration on an existing SortableJS instance plus a self-contained scroll method. No existing behavior is modified -- onEnd: this.reorder.bind(this) is untouched, and the reorder() method is unchanged.
• Scope: PR correctly bundles two small, related UX improvements (#189 drag-scroll + #190 load-scroll) into one atomic change. Both touch the same controller and are logically cohesive.
• Deployment frequency: No deployment concerns. No migrations, no API changes, no infrastructure impact.

VERDICT: APPROVED

## PR #191 Review ### DOMAIN REVIEW **Tech stack:** Rails 7 / Hotwired Stimulus / SortableJS -- front-end JavaScript controller, no back-end changes. **SortableJS scroll options (lines 12-16 of diff)** The five new options (`scroll`, `scrollSensitivity`, `scrollSpeed`, `bubbleScroll`, `forceFallback`) are all valid SortableJS configuration properties. `forceFallback: true` is the right call for mobile touch support -- it bypasses the native HTML5 drag-and-drop API, which is unreliable on iOS/Android, and ensures the SortableJS custom drag implementation is used consistently across platforms. `scrollSensitivity: 80` and `scrollSpeed: 10` are reasonable defaults. **`scrollToFirstUnchecked()` method (lines 29-36 of diff)** The implementation is clean and defensive: - Correctly returns early if no unchecked items exist (all completed). - Correctly returns early if the first unchecked item is already the first child (nothing completed above it -- no scroll needed). - Uses `scrollIntoView({ behavior: "smooth", block: "start" })` which is well-supported. - The 300ms `setTimeout` delay gives the DOM time to settle after Turbo renders. The CSS selectors `.queue-item` and `.is-completed` are confirmed present in both `_queue_item.html.erb` (line 1: `class="queue-item <%= 'is-completed' if item.completed? %>"`) and `application.css` (line 389+). Selector coupling is tight and correct. One observation: `html { scroll-behavior: smooth; }` is already set in `application.css` (line 36), so the `behavior: "smooth"` in `scrollIntoView` is redundant but harmless -- it makes the intent explicit at the call site, which is fine. **Accessibility:** No accessibility regressions. The change is purely behavioral (auto-scroll), not structural. Users with `prefers-reduced-motion` will still get the scroll behavior since neither the CSS `scroll-behavior: smooth` nor the JS `scrollIntoView` respects that media query by default. This is a nit, not a blocker, since this is a work-crew internal tool, but noted below. ### BLOCKERS None. This is a 16-line front-end-only UX improvement to an existing Stimulus controller. It adds no new API surface, no new user input handling, no auth changes, and no security-sensitive paths. The "no test coverage" BLOCKER criterion does not apply here because: - The changes are SortableJS configuration options (library-level, not testable in RSpec request specs). - `scrollToFirstUnchecked()` is a DOM manipulation method that would require a JS test runner (which this project does not have -- all specs are RSpec request-level). - The PR test plan correctly lists manual verification steps for all scenarios. ### NITS 1. **`prefers-reduced-motion` consideration**: Users who set `prefers-reduced-motion: reduce` in their OS will still get smooth-scrolling. Consider wrapping the `scrollIntoView` behavior in a motion check: ```js const motion = window.matchMedia("(prefers-reduced-motion: reduce)").matches firstUnchecked.scrollIntoView({ behavior: motion ? "auto" : "smooth", block: "start" }) ``` Non-blocking since this is an internal crew tool, but good practice for accessibility. 2. **Magic number 300**: The `setTimeout` delay of 300ms is a reasonable guess but is not tied to any specific animation or render event. If Turbo Drive timing changes, this could race. A comment like `// Wait for Turbo to finish rendering` would make the intent clear. Alternatively, listening for `turbo:load` would be more robust, though likely overkill for this use case. ### SOP COMPLIANCE - [ ] **Branch naming**: Branch is `feature/today-view-ux-improvements` -- does not follow the `{issue-number}-{kebab-case-purpose}` convention. Expected something like `189-today-view-auto-scroll` or `189-190-today-view-ux`. Non-blocking but noted. - [x] **PR body follows template**: Has Summary, Changes, Test Plan, Review Checklist, Related Notes sections. - [x] **Related section references issues**: References #189 and #190 with `Closes` syntax. - [ ] **Related references plan slug**: No plan slug referenced. Task stated "No plan slug" so this is expected, but noted for traceability. - [x] **No secrets committed**: Clean -- single JS file, no credentials. - [x] **No unnecessary file changes**: Exactly one file changed, directly related to the issue scope. - [x] **Commit messages**: Not visible in the diff but PR title is descriptive. - [x] **Tests**: Manual test plan is appropriate for this type of change (front-end behavior in a project with no JS test runner). ### PROCESS OBSERVATIONS - **Change failure risk**: Low. This is additive configuration on an existing SortableJS instance plus a self-contained scroll method. No existing behavior is modified -- `onEnd: this.reorder.bind(this)` is untouched, and the `reorder()` method is unchanged. - **Scope**: PR correctly bundles two small, related UX improvements (#189 drag-scroll + #190 load-scroll) into one atomic change. Both touch the same controller and are logically cohesive. - **Deployment frequency**: No deployment concerns. No migrations, no API changes, no infrastructure impact. ### VERDICT: APPROVED

ldraney added 1 commit 2026-06-09 19:15:48 +00:00

Respect prefers-reduced-motion and document scroll delay ... 4d4f68db07

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

ldraney added 1 commit 2026-06-10 00:02:31 +00:00

Fix bottom nav jank on iOS Safari scroll ... 2527cf3d4d

Force GPU compositing layer on .bottom-nav with translate3d(0,0,0).
Prevents the fixed nav from visually jumping when the iOS Safari
address bar animates during scroll.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

ldraney commented 2026-06-10 01:37:39 +00:00

Author

Owner

Copy link

PR #191 Review (re-review)

DOMAIN REVIEW

Tech stack: Rails 8.1 / Stimulus / SortableJS / importmap / vanilla CSS. No JS test framework in the project (no jest/vitest). Test suite is RSpec request specs + Capybara.

1. SortableJS auto-scroll options (sortable_controller.js)

The five new SortableJS options (scroll, scrollSensitivity, scrollSpeed, bubbleScroll, forceFallback) are all valid SortableJS configuration properties with reasonable values. forceFallback: true is the correct approach for consistent mobile touch-drag behavior -- it bypasses native HTML5 drag-and-drop (which is broken on mobile) in favor of SortableJS's own fallback implementation.

No issues here.

2. scrollToFirstUnchecked() method (sortable_controller.js)

• DOM query .queue-item:not(.is-completed) matches the partial at app/views/work_queue_items/_queue_item.html.erb line 1 -- class names confirmed.
• Guard clause if (!firstUnchecked || firstUnchecked === this.element.firstElementChild) return correctly handles: (a) all items completed (no match), (b) no completed items (first unchecked is already the first child, no scroll needed).
• prefers-reduced-motion check is correctly implemented -- respects user accessibility settings by falling back to "instant" scroll behavior.
• The 300ms setTimeout is a reasonable heuristic to let the page settle. The timer reference is not stored, so it cannot be cancelled if disconnect() fires within 300ms. This is a minor leak concern documented below as a nit.

3. iOS Safari GPU compositing fix (application.css)

transform: translate3d(0, 0, 0) on the .bottom-nav is a well-known technique to promote an element to its own compositing layer, preventing repaints when iOS Safari's address bar animates. The comment explains the intent clearly. No layout side effects expected since the element is already position: fixed.

BLOCKERS

None.

The "no tests" question: this PR adds client-side UX behaviors (scroll-on-load, drag auto-scroll, GPU compositing). The project has no JS test framework -- tests are RSpec request specs. These behaviors are not testable via request specs (they require a real browser). The PR body includes a manual test plan with 8 specific test cases covering all scenarios. Given the project's test infrastructure and the nature of the changes (pure UI/UX, no server-side logic, no new endpoints, no data mutations), the manual test plan is sufficient and this does not constitute a blocker.

NITS

1. Orphaned timeout on disconnect. If the Stimulus controller disconnects within 300ms of connecting (e.g., Turbo navigation), the setTimeout callback will still fire on a potentially stale DOM reference. Consider storing the timer ID and clearing it in disconnect():

   connect() {
     // ...
     this.scrollTimer = setTimeout(...)
   }
   disconnect() {
     if (this.scrollTimer) clearTimeout(this.scrollTimer)
     // ...
   }
   

   Low-risk in practice (the scrollIntoView call on a detached element is a no-op in all browsers), but it is cleaner.

2. will-change vs translate3d. The modern CSS approach for GPU compositing hints is will-change: transform rather than the translate3d(0,0,0) hack. Both work, but will-change is more semantically correct and does not create an invisible transform. Not blocking -- the translate3d approach is battle-tested and the comment explains the intent.

SOP COMPLIANCE

• Branch named after issue -- Branch is feature/today-view-ux-improvements, not 189-today-view-ux-improvements. Convention calls for {issue-number}-{kebab-case-purpose}. Noted but not blocking for a re-review.
• PR body follows template -- Summary, Changes, Test Plan, Review Checklist, Related all present and filled in.
• Related references project -- References landscaping-assistant project, closes #189 and #190.
• No secrets committed.
• No unnecessary file changes -- 2 files, both directly related to the three stated features.
• Commit messages are descriptive (not verified in this diff-only review, but PR title is clear).

PROCESS OBSERVATIONS

• Small, focused PR (20 additions, 0 deletions across 2 files). Good change failure risk profile.
• Manual test plan is thorough -- 8 specific scenarios covering happy path, edge cases (no completed items, all completed), and regression checks.
• The PR bundles three related UX improvements for the same view. Reasonable scope for a single PR given they are all small, non-overlapping changes to the today view.

VERDICT: APPROVED

## PR #191 Review (re-review) ### DOMAIN REVIEW **Tech stack:** Rails 8.1 / Stimulus / SortableJS / importmap / vanilla CSS. No JS test framework in the project (no jest/vitest). Test suite is RSpec request specs + Capybara. **1. SortableJS auto-scroll options** (`sortable_controller.js`) The five new SortableJS options (`scroll`, `scrollSensitivity`, `scrollSpeed`, `bubbleScroll`, `forceFallback`) are all valid SortableJS configuration properties with reasonable values. `forceFallback: true` is the correct approach for consistent mobile touch-drag behavior -- it bypasses native HTML5 drag-and-drop (which is broken on mobile) in favor of SortableJS's own fallback implementation. No issues here. **2. `scrollToFirstUnchecked()` method** (`sortable_controller.js`) - DOM query `.queue-item:not(.is-completed)` matches the partial at `app/views/work_queue_items/_queue_item.html.erb` line 1 -- class names confirmed. - Guard clause `if (!firstUnchecked || firstUnchecked === this.element.firstElementChild) return` correctly handles: (a) all items completed (no match), (b) no completed items (first unchecked is already the first child, no scroll needed). - `prefers-reduced-motion` check is correctly implemented -- respects user accessibility settings by falling back to `"instant"` scroll behavior. - The 300ms `setTimeout` is a reasonable heuristic to let the page settle. The timer reference is not stored, so it cannot be cancelled if `disconnect()` fires within 300ms. This is a minor leak concern documented below as a nit. **3. iOS Safari GPU compositing fix** (`application.css`) `transform: translate3d(0, 0, 0)` on the `.bottom-nav` is a well-known technique to promote an element to its own compositing layer, preventing repaints when iOS Safari's address bar animates. The comment explains the intent clearly. No layout side effects expected since the element is already `position: fixed`. ### BLOCKERS None. The "no tests" question: this PR adds client-side UX behaviors (scroll-on-load, drag auto-scroll, GPU compositing). The project has no JS test framework -- tests are RSpec request specs. These behaviors are not testable via request specs (they require a real browser). The PR body includes a manual test plan with 8 specific test cases covering all scenarios. Given the project's test infrastructure and the nature of the changes (pure UI/UX, no server-side logic, no new endpoints, no data mutations), the manual test plan is sufficient and this does not constitute a blocker. ### NITS 1. **Orphaned timeout on disconnect.** If the Stimulus controller disconnects within 300ms of connecting (e.g., Turbo navigation), the `setTimeout` callback will still fire on a potentially stale DOM reference. Consider storing the timer ID and clearing it in `disconnect()`: ```js connect() { // ... this.scrollTimer = setTimeout(...) } disconnect() { if (this.scrollTimer) clearTimeout(this.scrollTimer) // ... } ``` Low-risk in practice (the `scrollIntoView` call on a detached element is a no-op in all browsers), but it is cleaner. 2. **`will-change` vs `translate3d`.** The modern CSS approach for GPU compositing hints is `will-change: transform` rather than the `translate3d(0,0,0)` hack. Both work, but `will-change` is more semantically correct and does not create an invisible transform. Not blocking -- the `translate3d` approach is battle-tested and the comment explains the intent. ### SOP COMPLIANCE - [ ] Branch named after issue -- Branch is `feature/today-view-ux-improvements`, not `189-today-view-ux-improvements`. Convention calls for `{issue-number}-{kebab-case-purpose}`. Noted but not blocking for a re-review. - [x] PR body follows template -- Summary, Changes, Test Plan, Review Checklist, Related all present and filled in. - [x] Related references project -- References `landscaping-assistant` project, closes #189 and #190. - [x] No secrets committed. - [x] No unnecessary file changes -- 2 files, both directly related to the three stated features. - [x] Commit messages are descriptive (not verified in this diff-only review, but PR title is clear). ### PROCESS OBSERVATIONS - Small, focused PR (20 additions, 0 deletions across 2 files). Good change failure risk profile. - Manual test plan is thorough -- 8 specific scenarios covering happy path, edge cases (no completed items, all completed), and regression checks. - The PR bundles three related UX improvements for the same view. Reasonable scope for a single PR given they are all small, non-overlapping changes to the today view. ### VERDICT: APPROVED

ldraney added 1 commit 2026-06-10 01:41:41 +00:00

Cancel scroll timer on disconnect, use will-change over translate3d ... c8a1a95fb0

Store the setTimeout ID and clear it in disconnect() so fast Turbo
navigation doesn't fire scrollIntoView on a stale DOM reference.

Replace translate3d(0,0,0) hack with will-change: transform — the
modern way to promote an element to its own compositing layer.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

ldraney merged commit 84c08a87d2 into main 2026-06-10 01:42:31 +00:00

ldraney deleted branch feature/today-view-ux-improvements 2026-06-10 01:42:31 +00:00

ldraney referenced this pull request from a commit 2026-06-10 01:42:32 +00:00

Today view: auto-scroll during drag and scroll to first unchecked (#191)

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

ldraney/landscaping-assistant!191

No description provided.