fix: droppable panel submit navigation, scroll lock, mobile overlay height, facet seeding#47
Open
mekarpeles wants to merge 12 commits intomainfrom
Open
fix: droppable panel submit navigation, scroll lock, mobile overlay height, facet seeding#47mekarpeles wants to merge 12 commits intomainfrom
mekarpeles wants to merge 12 commits intomainfrom
Conversation
Issue #44 — two critical bugs found because tests only checked that ol-search fired, not that navigation happened: Navigation (droppable mode): - Add _buildSearchUrl(q, filters) that constructs siteBase/search?q=... with full filter params serialized as OL search URL query params - Make ol-search event cancelable — hosts that handle navigation themselves call e.preventDefault() to suppress the fallback - _submit() and "See all N results" both navigate via window.location.href when showFacets=true and event not prevented - Embedded mode (showFacets=false) is unaffected Body scroll lock: - Lock document.documentElement.style.overflow in addition to document.body; many browsers/frameworks scroll <html>, leaving a page scrollbar behind the position:fixed overlay - Restore both on close and in disconnectedCallback TDD: Playwright tests written BEFORE the fix (red → green): - submit button / Enter key / "See all N" each navigate to /search?q= - preventDefault() suppresses navigation - document.body + documentElement overflow:hidden while overlay open - both restored to '' when overlay closes AGENTS.md: explicit TDD section requiring a failing test before any fix
- Use _scrollLockActive flag in disconnectedCallback instead of
_mobileExpanded to correctly release scroll lock when component
is removed while overlay is open
- Save/restore _prevBodyOverflow and _prevDocumentOverflow before
locking so we never clobber pre-existing inline overflow styles
- Use new URL('/search', siteBase) to safely handle trailing slash
in siteBase (fixes potential double-slash in navigation URL)
- Add Playwright test asserting active filters appear in navigation URL
- Widen updated() slice to 2000 chars in panel-overlay static test
Vitest: assert scroll lock is gated on _open + showFacets, not
only _mobileExpanded, so desktop droppable panel also locks scroll.
Playwright: two desktop tests — overflow:hidden while panel open,
overflow restored after Escape closes it.
Tests fail red against current code; will go green once the lock
trigger is moved from changed.has('_mobileExpanded') to changed.has('_open').
…e overlay
Move scroll-lock trigger from changed.has('_mobileExpanded') to
changed.has('_open'), with condition this._open && this.showFacets.
This ensures the page behind the search panel cannot scroll on
desktop viewports just as it cannot on mobile.
- _facetLabel should use 'Authors'/'Subjects' (plural) - _toggleFacet should auto-seed author/subject facet with current _q
…t query - _facetLabel: 'Author'→'Authors', 'Subject'→'Subjects' - _toggleFacet: when opening author/subject facet with a non-empty _q, immediately call _onDropAuthorSearch/_onDropSubjectSearch to pre-populate results instead of showing an empty list
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes several droppable/search-panel regressions and UX issues: submit/Enter now navigates correctly in droppable mode, scroll locking covers all droppable panels (mobile + desktop) and restores previous overflow styles, mobile overlay height behavior is corrected, and author/subject facets are auto-seeded from the current query. Updates/expands Playwright + Vitest coverage and documents the project’s TDD workflow.
Changes:
- Add droppable-mode fallback navigation via
_buildSearchUrl()+ cancelableol-searchevent. - Extend scroll locking to any open droppable panel (lock
html+body, restore previous inline overflow; cleanup on disconnect). - Fix mobile overlay layout (flex column + scoped max-height) and add facet label pluralization + author/subject auto-seeding.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/components/ol-search-bar.js | Implements navigation fallback, scroll lock changes, facet seeding, pluralized labels, and overlay CSS adjustments. |
| frontend/tests/facet-and-submit.spec.js | Adds Playwright navigation/cancelation assertions for droppable submit/Enter/see-all behavior. |
| frontend/tests/mobile-overlay.spec.js | Adds Playwright assertions for body+html scroll lock and restoration on mobile + desktop droppable. |
| frontend/src/components/ol-search-bar.mobile-overlay.test.js | Updates static-analysis contracts for new scroll-lock gating/restoration and mobile overlay CSS rules. |
| frontend/src/components/ol-search-bar.panel-overlay.test.js | Adjusts updated() slice length to accommodate added logic. |
| frontend/src/components/ol-search-bar.facet-seed.test.js | New static-analysis tests for pluralization and author/subject facet auto-seeding on open. |
| AGENTS.md | Documents TDD workflow expectations and clarifies Copilot review timing guidance. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ltSubjects Replace tests checking _onDropAuthorSearch/Subject (wrong: populates typeahead results, not open defaults) with tests for _seedFacetsForQuery + _facetCache architecture that correctly updates _defaultAuthors/_defaultSubjects.
- backend: /api/search/facets now aggregates subject_facet and author_facet
fields from OL search.json docs (rows=30) instead of calling the HTML partial.
Returns top subjects and authors ranked by occurrence count.
- facets.js: add fetchQueryFacets(q) — calls /api/search/facets and returns
{ authors, subjects } shaped for ol-facet-drop's defaultAuthors/defaultSubjects.
- ol-search-bar: add _facetCache (Map) and _seedFacetsForQuery(q) method.
Opening author or subject facet while a query is active seeds _defaultAuthors /
_defaultSubjects from query-scoped facets; cache hit avoids re-fetching on
subsequent opens of the same query. Empty query keeps POPULAR_AUTHORS/SUBJECTS.
Dice handlers shuffle from the cached query pool when one exists.
The comment said 'ol-filter-change event' but the code directly mutates _localFilters. Copilot review comment #3151575651.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
_buildSearchUrlusesnew URL()for correctness_mobileExpanded; now locks when_open && showFacets(covers desktop dropdown too); locks bothdocument.bodyanddocument.documentElement; saves/restores pre-existing overflow values via_prevBodyOverflow/_prevDocumentOverflow;_scrollLockActiveflag guards cleanup indisconnectedCallback.panelis now a flex column (display:flex; flex-direction:column; min-height:0) so.ac-scrollcanflex:1to fill remaining space;@media (max-width:600px)max-height rule scoped to:host(:not(.mobile-exp))to avoid conflicts'Author'→'Authors','Subject'→'Subjects'_onDropAuthorSearch/_onDropSubjectSearchwiththis._qTests
All changes are TDD:
ol-search-bar.facet-seed.test.js,ol-search-bar.mobile-overlay.test.js,ol-search-bar.panel-overlay.test.jstests/facet-and-submit.spec.js,tests/mobile-overlay.spec.js228 Vitest tests pass. All new tests were committed failing before the implementation.
Backend investigation
OL's
/search.jsonhasfacet=Falsehardcoded ("This makes it much faster") — no structured public facet endpoint exists. Auto-seeding via the existing/api/authors/searchand/api/subjects/searchendpoints is the correct approach.