Put the shelf button on search results - #13400
Draft
lokesh wants to merge 5 commits into
Draft
Conversation
lokesh
force-pushed
the
feat/shelf-button-search-results
branch
2 times, most recently
from
August 22, 2026 12:09
ebd7b26 to
1923d02
Compare
Open
lokesh
force-pushed
the
feat/shelf-button-search-results
branch
from
August 22, 2026 16:19
1923d02 to
afb4ebc
Compare
Replaces my_books/dropper with <ol-shelf-button> on /search, behind a `use_shelf_button` flag on SearchResultsWork so the macro's five other callers stay on the dropper until each has been looked at on its own. The page is not user-cached, so shelf, rating and check-in state are server-rendered as attributes rather than fetched — but the buttons are stateless by contract, so the page also has to apply what they report. js/my-books/shelf-buttons.js does that, which is also what keeps two buttons for one work in step and what tells the check-in prompt the shelf moved. The prompt itself is untouched: it stays a sibling under the same `#check-in-container-<olid>` id the dropper used. get_patrons_reading_states() batches the three lookups the dropper did per result — the old path cost 2N queries for shelf and last-read-date alone. ILE's selection guard ignores clicks inside `a, button, details`. A click in a shadow root retargets to the host, so opening the popover was also selecting the row for the librarian toolbar. Labels come from my_books/book_actions_i18n.html, rendered once per request — every book on a page wants the same strings. Note that removing a book from a shelf deletes its check-ins server-side. The dropper warns first; this deliberately does not. Claude-Session: https://claude.ai/code/session_01XJ9QNXNg3UfbtH3dmDxUfV
lokesh
force-pushed
the
feat/shelf-button-search-results
branch
from
August 22, 2026 16:19
afb4ebc to
b30f427
Compare
The check-in pane now carries the answer it already holds: whichever quick row matches the recorded date is aria-current, "Other date" is a disclosure that opens seeded with a date no shortcut can express, and the Already Read row shows the date with a chevron — clicking it again reopens the pane to amend, passing event_id so the edit lands on the same check-in instead of counting a second finish. For January readers, the year just gone stays a one-tap answer for 30 days. With the popover asking, shelf_button.html stops rendering the sibling legacy prompt (check_in_prompt.html grows a show_prompt flag and CheckInComponents survives the missing element), and shelf-buttons.js keeps read-date/event-id in step across buttons the same way it does shelf and rating. Lists created in one popover, a sibling popover, or the legacy dropper now announce themselves via ol-list-created so every list pane on a mixed page stays honest without a refetch. Trending and author pages switch to use_shelf_button with the same batched reading-state lookup search results use. The rollout audit joins the repo as a working document, like its predecessor.
The shelf button now shares ol-button's type, raised surface, and hover fill and border. Since tapping Already Read again opens the date pane rather than unshelving, the popover gets a "Remove from shelf" row, and the rating row holds the same height as its neighbours so nothing shifts when a rating lands. Claude-Session: https://claude.ai/code/session_01RtTEd3rAHJYz3zKi3zUWwD
for more information, see https://pre-commit.ci
lokesh
added a commit
to lokesh/openlibrary
that referenced
this pull request
Aug 24, 2026
…ve#13400 Remove from shelf row, read-date display and amending (readDate/eventId through to setCheckIn's event_id), quickYears, the Other-date disclosure, the ol-list-created bridge and ol-book-check-in event, press feedback, fixed row heights, 16px list checkboxes, per-option select seeding, and the accent-color token fix — taken from the search-results branch so this PR carries the component work. Claude-Session: https://claude.ai/code/session_01RtTEd3rAHJYz3zKi3zUWwD
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
1 participant
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.
Stacked on #13399 — review that one first. This PR only adds the first surface.
What this does
Puts
<ol-shelf-button>on/search, replacingmy_books/dropperthere. It's behind ause_shelf_buttonflag onSearchResultsWork, so the macro's five other callers (trending, list pages, author works, fulltext results, reading log) stay on the dropper until each has been looked at on its own terms.Search results was chosen to go first because the page isn't user-cached:
render_cached_macroonly applies to macros wrapped inCacheableMacro, so shelf, rating and check-in state can be server-rendered as attributes with nothing to hydrate.But the buttons are stateless by contract — they emit
ol-book-state-changeand expect the surface to apply it. Server-rendering only supplies the opening state, sojs/my-books/shelf-buttons.jsowns it from there. That's also what keeps two buttons for one work in step, and what tells the check-in prompt the shelf moved.Check-ins are kept as they were. The prompt stays a sibling under the same
#check-in-container-<olid>id the dropper used, so no check-in code is touched.get_patrons_reading_states()batches the three lookups the dropper did per result. The old path cost 2N queries per page for shelf and last-read-date alone; twenty results meant forty round-trips before render.One fix that belongs to this surface
ILE's selection guard ignores clicks inside
a, button, details. A click in a shadow root retargets to the host, so opening the popover was also selecting the row for the librarian toolbar. The dropper's plain<button>matchedclosest(); ours doesn't.Three other bugs surfaced while building this — a rejected write reading as a save, the post-login resume target, and missing analytics — but they are component-level, so they moved down into #13399.
Labels
Labels come from
my_books/book_actions_i18n.html, rendered once per request and passed to each button. Every book on a page wants the same strings, and the blob is byte-identical, so repeating it costs ~3 bytes gzipped per extra instance.hide-rating(added in #13399) is not used here — the byline rating on search results is aggregate display, not an input, so there is nothing to duplicate. The reading log will need it.Testing
Unit tests for the new component behaviour, the batched helper, and the state owner. Plus
tests/e2e/shelf-button.spec.ts— the state-owner bug and the ILE bug both lived in the seam between server-rendered attributes and a component that upgrades later, which is invisible to unit tests.Manually verified in dev, both auth states: server-rendered state, shelving from the main half, rating (which moves the book to Already Read and reveals the check-in prompt), add-to-list, and the signed-out redirect with the intent cookie.
Three pre-existing failures in
test_addbook.py/test_models.pyare unrelated — they fail on the base branch too.Known, and deliberate
Removing a book from a shelf deletes its check-ins server-side (
api.py:159). The legacy dropper warns before doing that; this does not. Called deliberately — the alert wasn't worth the interruption — but worth knowing when comparing the two paths.Not in this PR
Fulltext results are deliberately left out and want a look of their own: it passes an edition doc with no work key readily available, so it likely falls into the dropper's
old-style-listspath today (lists only, no shelves). Adding shelves there is a feature change, not a swap.https://claude.ai/code/session_01XJ9QNXNg3UfbtH3dmDxUfV