Skip to content

feat: add sticky inputs section to gaps and timeline pages - #1528

Open
eran132 wants to merge 8 commits into
mainfrom
feat/sticky-inputs
Open

feat: add sticky inputs section to gaps and timeline pages#1528
eran132 wants to merge 8 commits into
mainfrom
feat/sticky-inputs

Conversation

@eran132

@eran132 eran132 commented Apr 24, 2026

Copy link
Copy Markdown
Collaborator

Re-submitted from upstream branch (previously #1513).

Summary

Created reusable StickyInputs component. Applied to gaps and timeline pages.

Closes #1059

Test plan

  • TypeScript compiles clean
  • ESLint passes
  • Unit tests pass (9/9)
  • CI checks pass

🤖 Generated with Claude Code

Created a reusable StickyInputs component that keeps the filter
controls visible when scrolling down through results. Applied to
the gaps page and historic timeline page where long result tables
cause the inputs to scroll out of view.

Closes #1059

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings April 24, 2026 23:21
@eran132
eran132 requested a review from AvivAbachi as a code owner April 24, 2026 23:21

Copilot AI 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.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds a reusable sticky header wrapper for input controls and applies it to the Gaps and Historic Timeline pages so the filters remain visible while scrolling.

Changes:

  • Created a reusable StickyInputs styled-component wrapper.
  • Wrapped the input/filter sections on the historic timeline and gaps pages with StickyInputs.
  • Adjusted layout structure so only the input section is sticky while results/content scroll.

Reviewed changes

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

File Description
src/pages/historicTimeline/index.tsx Wrapes the filter inputs in StickyInputs to keep them visible while scrolling the timeline content.
src/pages/gaps/index.tsx Wrapes the filter inputs in StickyInputs so filters stay visible above the gaps table.
src/pages/components/StickyInputs.tsx Introduces the reusable sticky wrapper component (positioning, layering, background).

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

import OperatorSelector from 'src/pages/components/OperatorSelector'
import RouteSelector from 'src/pages/components/RouteSelector'
import { Row } from 'src/pages/components/Row'
import { StickyInputs } from 'src/pages/components/StickyInputs'

Copilot AI Apr 24, 2026

Copy link

Choose a reason for hiding this comment

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

Import style for StickyInputs is inconsistent across pages: this file uses an absolute src/... import while src/pages/gaps/index.tsx uses a relative ../components/StickyInputs. Standardizing on one convention improves consistency and avoids path-alias-only builds causing import churn (and can prevent accidental duplicate module instances in some setups).

Suggested change
import { StickyInputs } from 'src/pages/components/StickyInputs'
import { StickyInputs } from '../components/StickyInputs'

Copilot uses AI. Check for mistakes.
Comment thread src/pages/components/StickyInputs.tsx Outdated
Comment on lines +6 to +8
z-index: 100;
background: inherit;
padding-bottom: 8px;

Copilot AI Apr 24, 2026

Copy link

Choose a reason for hiding this comment

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

The z-index: 100 and padding-bottom: 8px are hard-coded magic numbers. Consider deriving these from the app/theme system (e.g., theme spacing and z-index scale) so stacking and spacing stay consistent across pages and don’t require manual tuning when global layout changes.

Suggested change
z-index: 100;
background: inherit;
padding-bottom: 8px;
z-index: var(--z-index-sticky-inputs, 100);
background: inherit;
padding-bottom: var(--spacing-sticky-inputs-bottom, 8px);

Copilot uses AI. Check for mistakes.
Comment thread src/pages/components/StickyInputs.tsx Outdated
position: sticky;
top: 0;
z-index: 100;
background: inherit;

Copilot AI Apr 24, 2026

Copy link

Choose a reason for hiding this comment

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

background: inherit can resolve to transparent if the parent doesn’t set a background, which can reduce contrast/readability when the sticky section overlays scrolling content. Prefer setting an explicit background color (typically the page/surface background from the theme) and optionally a subtle border/shadow to clearly separate the sticky inputs from the content underneath.

Suggested change
background: inherit;
background: #fff;
border-bottom: 1px solid rgba(0, 0, 0, 0.08);

Copilot uses AI. Check for mistakes.
@github-actions

github-actions Bot commented Apr 24, 2026

Copy link
Copy Markdown
Contributor

@eran132
eran132 requested a review from NoamGaash April 25, 2026 07:11
@NoamGaash
NoamGaash removed their request for review April 25, 2026 16:56

@NoamGaash NoamGaash 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.

Please verify (human) that the new layout looks good - this issue is about appearance and user experience

Image

eran132 and others added 2 commits May 18, 2026 17:51
# Conflicts:
#	src/pages/historicTimeline/index.tsx
background: inherit resolved to transparent (PageContainer has no
background), so scrolling results bled through the pinned inputs in
both light and dark mode. Use MUI theme palette.background.default
so the sticky bar matches the page background ScopedCssBaseline paints.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@eran132

eran132 commented May 18, 2026

Copy link
Copy Markdown
Collaborator Author

@NoamGaash — verified the layout manually as you asked, and good thing I did: while testing the scroll behavior with real data loaded on both /timeline and /gaps, I found the pinned inputs had a transparent background (background: inherit resolved to transparent because PageContainer has no background). Scrolling results bled straight through the inputs and made them unreadable — in both light and dark mode.

Fixed in 7261786: StickyInputs now uses the MUI theme's palette.background.default (the same color ScopedCssBaseline paints the page), so the sticky bar is opaque and theme-correct.

Verified after the fix, both pages, both themes, RTL:

  • Inputs stay pinned at top while results scroll underneath ✅
  • Solid background, no bleed-through, fully readable ✅
  • Light + dark both clean ✅

Also rebased on main — there was a conflict in historicTimeline/index.tsx (main refactored the timestamp logic to .startOf('minute') and removed a StyledTimelineBoard reference that was never defined). Kept main's newer logic + the imported TimelineBoard.

tsc + eslint + tests all green. Re-requesting review.

@NoamGaash NoamGaash 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.

thank you! looks great

# Conflicts:
#	src/pages/gaps/index.tsx
#	src/pages/historicTimeline/index.tsx
@eran132

eran132 commented Jun 2, 2026

Copy link
Copy Markdown
Collaborator Author

Resolved the conflict with main (commit 11b0992). Main had refactored the timeline + gaps pages substantially (new `date`/`timeOfDay` state, timezone-aware `dayjs.tz(date, ISRAEL_TIMEZONE)`, `?? undefined` null-safety, `search.stopKey` from context, new computed `time` for `TimelineBoard`). I kept the `` wrapper around the input rows, adopted all of main's improvements inside it, and kept the hits-timeline outside the wrapper so results still scroll under the pinned bar (the whole point of this PR).

Net diff vs current main is still small and focused: 3 files (+162/-141) — `StickyInputs.tsx`, `gaps/index.tsx`, `historicTimeline/index.tsx`.

Verified: `tsc` ✅, full lint ✅, unit tests 64/64 ✅, plus Playwright + visual review of sticky-scroll behavior on timeline (light + dark) and gaps (light) — inputs stay pinned at top with opaque themed background (the bug fixed earlier in this PR), results scroll cleanly underneath, all inputs populate correctly with main's new state shape.

@NoamGaash NoamGaash 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.

seems like there are few visual regressions

eran132 and others added 2 commits June 6, 2026 13:56
NoamGaash flagged a visual regression on the gaps page: the always-opaque
sticky bar showed as a faint container box around the inputs at rest,
because StickyInputs' background.default didn't match the surrounding
PageContainer color exactly on that page.

Fix: paint the background ONLY when the scrollable ancestor has actually
scrolled (scrollTop > 0). That's exactly when content would otherwise
bleed through the sticky bar (the original #1528 fix). At rest there's
nothing behind the bar to hide, so transparency matches the pre-PR
visual baseline pixel-for-pixel.

Detection walks up the DOM for the nearest scrollable ancestor (overflowY
auto/scroll/overlay) and listens to its scroll. This works for this
codebase's inner-scrollable layout; falls back to window scroll if none
found. Logic also asserted via Playwright (data-stuck='true' after wheel).

Verified visually: gaps-dark at rest is now pixel-identical to main (no
box); timeline-dark scrolled has solid bg with no bleed-through (station
name reads cleanly, no timestamps overlapping the Hebrew text).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@eran132

eran132 commented Jun 6, 2026

Copy link
Copy Markdown
Collaborator Author

@NoamGaash you were right — there was a real regression. Investigated and fixed in 27fba87.

What was wrong: the always-opaque sticky bar (which I'd added earlier to stop scrolling timeline content from bleeding through it) was being painted with theme.palette.background.default. On the gaps page that color doesn't exactly match the surrounding PageContainer area, so at rest the input section showed up as a faint container box — clearly visible in dark mode against the near-black page. On the timeline page it happens to match, which is why I missed it in my own visual check.

The fix: conditional opacity. The bar is transparent at rest, becomes opaque only when its scrollable ancestor has actually scrolled (scrollTop > 0). The two requirements are opposite by nature:

  • At rest there's nothing behind the bar to hide — transparency guarantees zero visual diff vs main on any page, regardless of which container surrounds it
  • Scrolled, content IS behind it — opacity is needed to stop the bleed-through (the bug we fixed earlier)

Detection walks up the DOM for the nearest scrollable ancestor (overflow-y auto/scroll/overlay) and listens to its scroll event. Works for this codebase's inner-scrollable layout; falls back to window scroll if no inner scroller is found.

Verified locally (via my own Playwright + frame inspection, since I don't have Applitools access):

  • At-rest gaps-dark is now pixel-identical to main — no visible container box
  • Scrolled timeline-dark has solid opaque bg — station name חיים הרצוג/שדרות מנחם בגין (גדרה) reads cleanly with no timeline timestamps bleeding through it
  • Playwright assertion: data-stuck === 'true' after wheel-scroll (deterministic, not just visual)

Re-requesting review. Applitools should also be happy this time — at-rest baselines should match. Let me know if the diffs look right on your end.

@NoamGaash

NoamGaash commented Jun 6, 2026

Copy link
Copy Markdown
Member

what's your email address? I'll add you.
I think that the Eyes dashboard is public, so you should have read-only permissions even without me adding you (tho I do want to add you so you'll be able to execute and accept new baselines)

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.

style enhancement: introduce sticky inputs section

3 participants