diff --git a/BOOK-ACTIONS-ROLLOUT-AUDIT.md b/BOOK-ACTIONS-ROLLOUT-AUDIT.md new file mode 100644 index 00000000000..1b2d6d7d8bf --- /dev/null +++ b/BOOK-ACTIONS-ROLLOUT-AUDIT.md @@ -0,0 +1,242 @@ +# Rolling `ol-shelf-button` / `ol-book-actions` into the product + +Audit of what it would take to make the new book-action components the standard +way readers shelve, rate, and list books — and what breaks if we do it naively. + +Branch audited: `feat/shelf-button-book-actions` (components) plus +`feat/books-display-kit` (the carousel kit, draft PR #13381). + +--- + +## 1. What we have + +Three components, all already exported from `openlibrary/components/lit/index.js`, +which `site/footer.html` loads as a module on **every page**. So the bundle cost is +paid the moment the components PR merges; rolling them out onto surfaces adds no +further JS weight (~8KB gzipped, minified, for all three plus `books-api.js`). + +| Component | Role | +|---|---| +| `ol-book-actions` | The popover: four shelf rows, a 5-star rating, and a sliding "Add to list" pane with filter + inline create. Caller supplies the trigger via `slot="trigger"`. | +| `ol-shelf-button` | The trigger, in two shapes: `split` (bordered, labelled, main half toggles Want to Read) and `icon` (round bookmark for floating over cover art). | +| `ol-book-cover` | Cover art with an `overlay` slot, which is where the icon variant lives. | + +**Contract worth preserving.** Both controls are stateless — they never write their +own `shelf`/`rating`, they emit `ol-book-state-change` (optimistically, and again +with the old value on failure) and the owning surface applies it. That is what keeps +two cards for the same work in step, and it makes the optimistic write and its +rollback one code path. Any host we add them to has to own that state. + +**Data plumbing.** `GET /reading-state.json?work_ids=OL1W,OL2W` (FastAPI, new on this +branch) returns `{shelves: {...}, ratings: {...}}` for the signed-in reader. Server- +rendered surfaces can skip it and set `shelf`/`rating` directly; client-rendered ones +(carousels, partial-loaded results) need the batch call. + +--- + +## 2. What is in production today + +The legacy stack, all rendered through `openlibrary/templates/my_books/dropper.html`: + +- `my_books/primary_action.html` — the "Want to Read" `
` button +- `my_books/dropdown_content.html` — shelf forms, list checkboxes, "Use this Work", create-list modal +- `my_books/check_ins/check_in_prompt.html` — "Read on " + edit, rendered as a **sibling** of the dropper +- `macros/StarRatings.html` — a separate five-star form, not part of the dropper +- `js/my-books/MyBooksDropper.js` + `lists/ShowcaseItem.js` — behaviour and the sidebar list chips + +Where it appears: + +| Surface | Template | Dropper | Separate stars | +|---|---|---|---| +| Work / edition page | `macros/databarWork.html` → `lists/widget.html` | yes | yes (with schema.org RDFa) | +| Search results | `macros/SearchResultsWork.html` (`work_search.html`) | yes (async) | byline only | +| List pages | `type/list/view_body.html` | yes | — | +| Author works | `type/author/view.html` | yes | — | +| Trending | `trending.html` | yes | — | +| Reading log | `account/reading_log.html` | yes (own pages) | yes | +| Loan history | `account/loan_history.html` | — | yes | +| Fulltext results | `macros/FulltextResults.html` | yes | — | +| Author page sidebar | `type/author/view.html` → `lists/widget.html` | yes (author seed, no work) | — | +| **Carousels** | `books/custom_carousel_card.html` | **none** | **none** | + +One useful de-risking fact: the legacy primary-action button is server-rendered +`disabled` and enabled by JS. There is **no no-JS baseline to protect** — a Lit +component is no regression on that axis. + +--- + +## 3. Feature-parity gaps + +Ranked by how much they hurt. + +### 3.1 Blockers — fix before any production surface + +**i18n.** Every string in both components is an English default in `DEFAULT_LABELS`. +Nothing on the server passes `labels`. OL is heavily translated; shipping this to a +real surface ships English to every locale. The patterns to fix it already exist in +the codebase — the work is picking the right one and wiring it. See §5.1. + +**Check-in destruction, silently.** `process_work_bookshelves` calls +`BookshelvesEvents.delete_by_username_and_work` whenever a book comes off a shelf +(`openlibrary/plugins/openlibrary/api.py:159`). The legacy dropper confirms first — +*"Removing this book from your shelves will delete your check-ins for this work. +Continue?"*. `ol-shelf-button`'s main half and the popover's shelf rows both remove +without asking. On any surface where a reader has check-ins, this is quiet data loss. + +**`{"error": "Invalid bookshelf"}` returns HTTP 200.** `books-api.js`'s `request()` +only throws on `!response.ok`, so a rejected shelf write resolves successfully, the +optimistic UI sticks, and the reader believes it saved. This is exactly the failure +shape behind the dev-only "Invalid bookshelf" bug already in the notes. Needs a body +check, not just a status check. + +### 3.2 Real gaps — surface-dependent + +| Capability | Legacy | New | Matters on | +|---|---|---|---| +| Check-in prompt ("Read on 3 May") | yes | no | work page, search, list, reading log | +| Confirm before deleting check-ins | yes | **no** | everywhere signed-in | +| "Use this Work" — edition vs work seed for lists | yes | no (always uses `book.key`, the work) | edition pages | +| Sidebar list showcase chips update on add | yes | no | work page | +| Author / subject seeds (list-only, no work) | yes | no | author page sidebar | +| Create list with a description | yes | name only | minor | +| `data-ol-link-track` analytics | yes | no | all — see §5.3 | +| Login-intent preservation | yes | yes (`queuePendingAction`) | — | +| schema.org `reviewRating` RDFa | yes (`StarRatings.html`) | no | **work page SEO** | + +The check-in gap is smaller than it looks: `check_in_prompt.html` is already a +sibling of the dropper, found by `document.querySelector('#check-in-container-')`. +A ~30-line adapter that listens for `ol-book-state-change` and opens/hides the +existing prompt gets us parity without touching check-in code. + +### 3.3 Scaling concerns + +- `/partials/MyBooksDropperLists.json` returns **every seed key of every one of the + user's lists** (`get_list_data` → `list_items`). A reader with a 5,000-book list + ships that whole array. The new component prefetches it on the *first popover open* + and shares one promise page-wide, which is no worse than the legacy dropper — but + it will now fire on surfaces that never loaded it before. +- `/reading-state.json` caps at 100 work ids (`MAX_STATE_WORKS`); over that is a 422. + Long carousels or a 100+ item list page need chunking. +- A page with both the legacy dropper and a new popover has **two list caches** + (`myBooksStore` and the module-level `_listsPromise`). ~~Toggling a list in one + leaves the other stale.~~ **Wired** (author-page rollout): the only cross-talk + that matters is list *creation* — membership state doesn't cross seeds — and + both sides now announce it with an `ol-list-created` DOM event. The popover + dispatches it (bubbling) after its inline create; `CreateListForm` dispatches + it on `document`; each side folds the other's creations into its own cache. + This also fixed a sibling bug: `_onCreateSubmit` replaced the lists object, so + already-loaded sibling popovers never saw a newly created list. + +--- + +## 4. Status + +**Shipped:** search results, PR #13400 (stacked on #13399). Then **trending and +author works** — both were the predicted flag flip plus the page-level +`get_patrons_reading_states` batch, with one addition each worth recording: +trending can list the same work twice (`shelf-buttons.js` already keeps +duplicate buttons in step), and the author page is the first **mixed page** — +its sidebar keeps the legacy author-seed dropper — which forced wiring the +list-cache bridge described in §3.3. + +`SearchResultsWork` gained a `use_shelf_button` flag; `work_search.html` turns it +on. Everything else still renders `my_books/dropper`. + +### What made search results the right first surface + +Not that it was lowest-risk in the abstract — carousels are — but that it is +**not user-cached**. `render_cached_macro` only applies to macros wrapped in +`CacheableMacro`, so shelf, rating and check-in state server-render as +attributes with nothing to hydrate. All six `SearchResultsWork` callers share +that property; carousels do not. + +One thing server-rendering does *not* solve, which cost a real bug: the buttons +are stateless by contract, so the page still has to apply what they report. +`js/my-books/shelf-buttons.js` is that owner. Server-rendering supplies only the +opening state. + +### Remaining surfaces + +| Surface | What it still needs | +|---|---| +| **Reading log** | `hide-rating` — the only surface where `macros.StarRatings` is a real input. Shelf is implicit from the page (`/want-to-read`), so it needs no shelf query at all, and `ratings[idx]` is already batched. Owner-only (`include_dropper=(bookshelf_id and owners_page)`). | +| **List pages** | Edition seeds render `doc = seed.document`, so `doc.key` is `/books/OL…M` and `work-key` needs `doc.works[0].key`. Also `decorations=remove_item_link()` already offers remove-from-this-list, which the popover's list pane would then offer twice on one row. | +| **Fulltext results** | **Deliberately deferred — needs its own look.** It passes `doc['edition']`, a search-index edition doc with no work key readily available, so it likely falls into the dropper's `old-style-lists` path today: lists only, **no shelves at all**. Adding shelves there is a feature change, not a swap, and wants a Solr work-key lookup. | +| **Author page sidebar** | Not a candidate. `lists/widget.html` renders a dropper for an *author* seed with no work key; `ol-book-actions` is work-shaped. | +| **Carousels** | Blocked on a state-hydration story. Cards are `CacheableMacro`-cached across users and lazy-loaded via `CarouselCardPartial`, so `user-key`/`shelf`/`rating` cannot be baked in. `ol-books-display` (draft PR #13381) was going to own this but is not merged; the alternative is the existing Templetor card plus a hydration controller. | +| **Work / edition page** | Last. Showcase chips, the edition-vs-work list seed, mobile modal links, and `StarRatings.html`'s schema.org RDFa (keep it — the popover runs with `hide-rating`). | + +## 5. Component API + +### 5.1 Label plumbing — settled: pattern B + +Three patterns exist in the codebase: + +| Pattern | Used by | Shape | +|---|---|---| +| **A.** Individual `label-*` attributes | `ol-pagination` (5), `ol-carousel` (4), `ol-scorecard` (7), `ol-read-more` (2) | Lit, one instance per page | +| **B.** One JSON blob on the instance | search-bar trigger, `ReturnForm`, `login`, `history` | self-describing instance | +| **C.** One `render_once()` blob per page, queried by JS | `list-i18n-strings`, `reading-log-i18n-strings` | many instances share one blob | + +**B was chosen.** 23 label keys, 773 bytes of JSON, 1,233 HTML-escaped. The +obvious objection — repeating it per card — does not survive measurement: + +| | raw | gzipped | +|---|---|---| +| 20 instances | +24.9KB | **+627B** | +| 120 instances | +149KB | **+1.6KB** | + +Every instance carries byte-identical JSON, so after the first each repeat costs +about **3 bytes** on the wire. B also needs no component change (`labels: { type: +Object }` already JSON-parses the attribute) and composes with async-loaded +markup, which C does not. + +Implemented as `my_books/book_actions_i18n.html`, rendered once per request in +`work_search.html` and passed down. `ol-book-cover` has one label (`by %(name)s`) +and is genuinely pattern A territory. + +### 5.2 Added in #13400 + +- `hide-rating` — drops the popover's stars. +- `has-check-in` — a removal would destroy something, so ask first. +- `get_patrons_reading_states()` — batches shelf, rating and last-read-date. +- `trackEvent` for the Lit bundle, keeping the `ReadingLog|*` names. + +### 5.3 Still worth doing + +- **`seed-key`.** `_seedKey` is still hardcoded to `book.key`, so an edition page + cannot add the edition to a list and "Use this Work" has nowhere to live. + Blocks list pages and the book page. +- **A leaner list-membership endpoint.** `/partials/MyBooksDropperLists.json` + returns every seed key of every one of the user's lists. + +## 6. Bugs found and fixed in #13400 + +1. `bookshelves.json` answers a rejected write with 200 and an `error` key; + `books-api`'s status-only check let a failed write look like a save. +2. Removing a shelf deletes the work's check-ins server-side; the components did + not warn, the dropper did. Now warns only when there is one to lose. +3. ILE's selection guard ignores clicks inside `a, button, details`, but a click + in a shadow root retargets to the host — so opening the popover also selected + the row for the librarian toolbar. +4. Signed out, the button resumed on the book's page after login rather than the + page the reader was on. +5. Nothing applied the state the (deliberately stateless) buttons reported, so + the label stopped matching the server after the first change. + +Bugs 3 and 5 lived in the seam between server-rendered attributes and a component +that upgrades later — invisible to unit tests, which is why +`tests/e2e/shelf-button.spec.ts` exists. + +## 7. Open questions + +1. Should list membership move off `/partials/MyBooksDropperLists.json` to a + leaner endpoint (`{key, name, count, contains}` for one seed) before this + reaches higher-traffic surfaces? +2. A signup interstitial for logged-out readers ("sign up to save this") instead + of a bare login bounce. Cheaper after the migration than before it — one + change in `_onLoggedOut` rather than one per surface. +3. On the reading log, removing a book from the shelf you are looking at leaves a + row that is now lying. Left as-is for now; treatment to be explored. +4. Does `ol-books-display` (#13381) land before carousels are attempted, or do + carousels go direct on the Templetor card with a hydration controller? diff --git a/openlibrary/components/lit/OlBookActions.js b/openlibrary/components/lit/OlBookActions.js index 3eec4757645..4be502be4ec 100644 --- a/openlibrary/components/lit/OlBookActions.js +++ b/openlibrary/components/lit/OlBookActions.js @@ -19,6 +19,7 @@ export const DEFAULT_LABELS = { currentlyReading: 'Currently Reading', alreadyRead: 'Already Read', stoppedReading: 'Stopped Reading', + removeFromShelf: 'Remove from shelf', rateThisBook: 'Rate this book', rateStar: 'Rate %(rating)s of 5', clearRating: 'Clear rating', @@ -76,6 +77,40 @@ function MONTHS() { return _months; } +/** + * A check-in date for display. The schema stores partial dates, so "2026", + * "2026-08" and "2026-08-22" are all valid and each shows only what is known. + */ +function formatReadDate(value) { + const [year, month, day] = String(value).split('-').map(Number); + if (!year) return ''; + const lang = document.documentElement.lang || 'en'; + const options = month + ? (day ? { year: 'numeric', month: 'short', day: 'numeric' } : { year: 'numeric', month: 'short' }) + : null; + if (!options) return String(year); + return new Intl.DateTimeFormat(lang, options).format(new Date(year, month - 1, day || 1)); +} + +/** + * The years offered as one tap. For the first 30 days of a new year the year + * just gone stays on offer: that is when a reader is most likely logging + * something they finished before the turn, and "In 2025" on 25 January saves + * them the date picker. + */ +export function quickYears(now = new Date()) { + const year = now.getFullYear(); + const daysIn = Math.floor((now - new Date(year, 0, 1)) / 86400000); + return daysIn < 30 ? [year, year - 1] : [year]; +} + +/** The inverse: `{year, month, day}` as the schema stores it. */ +function partialDate({ year, month, day }) { + const pad = n => String(n).padStart(2, '0'); + if (!month) return String(year); + return day ? `${year}-${pad(month)}-${pad(day)}` : `${year}-${pad(month)}`; +} + // One in-flight lists request shared by every popover on the page. let _listsPromise = null; /** Drop the shared lists cache (tests, or after a mutation elsewhere). */ @@ -98,12 +133,23 @@ export function resetListsCache() { * @prop {Object} book - `{ key, title, firstPublishYear?, editionKey? }` * @prop {Number} shelf - Current shelf id (1–4) or null * @prop {Number} rating - Current rating (1–5) or null + * @prop {String} readDate - The check-in date, whole or partial ("2026", + * "2026-08", "2026-08-22"), or null when the reader has not given one + * @prop {Number} eventId - Id of that check-in, so changing the date edits it + * rather than recording a second finish * @prop {String} userKey - "/people/", needed to create lists * @prop {Object} labels - Translated strings (see DEFAULT_LABELS) * @prop {String} placement - ol-popover placement; unset uses its default * * @fires ol-book-state-change - After a shelf or rating change is accepted by * the server. detail: { key, shelf, rating } + * @fires ol-book-check-in - After a finish date is accepted by the server, so + * the surface can hand it back. detail: { key, date, eventId } — `date` is + * whole or partial, as stored. + * @fires ol-list-created - After the inline form creates a list, so sibling + * popovers and any legacy droppers on the page can add the row. The legacy + * side dispatches the same event on `document` when it creates one. + * detail: { key, name, seedKey } * * @slot trigger - The button that opens the popover. */ @@ -112,6 +158,8 @@ export class OlBookActions extends LitElement { book: { type: Object }, shelf: { type: Number }, rating: { type: Number }, + readDate: { type: String, attribute: 'read-date' }, + eventId: { type: Number, attribute: 'event-id' }, userKey: { type: String, attribute: 'user-key' }, labels: { type: Object }, placement: { type: String }, @@ -141,6 +189,9 @@ export class OlBookActions extends LitElement { /* A fixed measure: the popover shrink-wraps its content, and the title would otherwise size the panel per book. */ width: 300px; + /* One height for every row, so the panel never shifts as rows + re-render (the rating caption swaps between a span and a button). */ + --_row-height: calc(var(--font-size-body-medium) * var(--line-height-body) + 2 * var(--spacing-inset-sm)); /* Keeps the first and last rows off the rounded corners. */ padding-block: var(--spacing-inset-xs); color: var(--color-text); @@ -256,6 +307,7 @@ export class OlBookActions extends LitElement { align-items: center; gap: var(--spacing-inline-md); box-sizing: border-box; + min-height: var(--_row-height); margin: 0; margin-inline: var(--spacing-inset-xs); padding-block: var(--spacing-inset-sm); @@ -297,6 +349,32 @@ export class OlBookActions extends LitElement { } } + /* Press feedback, the same tactile squeeze gives: colour + changes are instant, only the scale animates. A row has no resting + fill, so the press paints one — on touch, where :hover never runs, + there would otherwise be nothing to squeeze. */ + .row, + .list-row { + transition: transform 0.08s; + } + + .row:active, + .list-row:active { + background: var(--color-hover-overlay); + transform: scale(0.97); + } + + /* Except the shelf rows: clicking one re-renders it — label weight and + colour change, a check mark appears — and re-laying out mid-scale + reads as a flicker. They keep the press fill, not the squeeze. */ + .group.shelves .row { + transition: none; + } + + .group.shelves .row:active { + transform: none; + } + .row:focus-visible { outline: 2px solid var(--color-focus-ring); outline-offset: -2px; @@ -321,7 +399,14 @@ export class OlBookActions extends LitElement { display: flex; align-items: center; gap: var(--spacing-inline-md); - padding: var(--spacing-inset-sm) var(--spacing-inset-md); + box-sizing: border-box; + height: var(--_row-height); + padding: 0 var(--spacing-inset-md); + } + + .star-buttons, + .stars .caption { + line-height: 1; } .star-buttons { @@ -349,6 +434,16 @@ export class OlBookActions extends LitElement { color: var(--gold); } + /* Icon-only, so 3% would be sub-pixel — presses its icon + shapes harder for the same reason. */ + .star { + transition: transform 0.08s; + } + + .star:active { + transform: scale(0.93); + } + .star:focus-visible { outline: 2px solid var(--color-focus-ring); border-radius: var(--border-radius-sm); @@ -388,6 +483,28 @@ export class OlBookActions extends LitElement { font-size: var(--font-size-label-medium); } + /* A disclosure, not a link onwards: the chevron points down at the + fields the row opens and flips once they are showing. */ + .date-toggle .trail { + transition: transform 180ms cubic-bezier(0.165, 0.84, 0.44, 1); + } + + .date-toggle[aria-expanded='true'] .trail { + transform: rotate(180deg); + } + + @media (prefers-reduced-motion: reduce) { + .date-toggle .trail { + transition: none; + } + } + + /* Sits directly under the row that opened it, so the gap reads as a + seam between row and fields rather than a new section. */ + .date-form { + padding-top: var(--spacing-inset-xs); + } + /* Three selects on one line only fit at small size — the same height and radius the small web-component controls use. */ .date-fields { @@ -432,6 +549,17 @@ export class OlBookActions extends LitElement { } /* Lists pane */ + + /* Header and field each hold a small control at most, and swap what + they show when creating a list; a fixed height keeps the list below + from jumping when they do. */ + .lists-header, + .pane-header, + .field { + box-sizing: border-box; + height: calc(var(--control-height-small) + 2 * var(--spacing-inset-sm)); + } + .lists-header, .pane-header { position: relative; @@ -462,8 +590,9 @@ export class OlBookActions extends LitElement { .field { display: flex; + align-items: center; gap: var(--spacing-inline-sm); - padding: var(--spacing-inset-sm) var(--spacing-inset-md); + padding: 0 var(--spacing-inset-md); margin-bottom: var(--spacing-stack-xs); } @@ -508,12 +637,13 @@ export class OlBookActions extends LitElement { } } - /* 20px like the main pane's row icons, so both panes share one row - height and one label column. */ + /* 16px like the other popover controls, but sitting in a 20px slot so + it lines up with the main pane's row icons — one row height, one + label column across both panes. */ .list-row input { - width: 20px; - height: 20px; - margin: 0; + width: 16px; + height: 16px; + margin-inline: 2px; accent-color: var(--color-primary); flex: 0 0 auto; } @@ -651,9 +781,15 @@ export class OlBookActions extends LitElement { > ${this.t(row.label)} - ${this.shelf === row.id ? html`` : nothing} + ${this._renderShelfTrail(row)} `)} + ${this.shelf ? html` + + ` : nothing} ${this.hideRating ? nothing : html`
@@ -671,6 +807,21 @@ export class OlBookActions extends LitElement { `; } + /** + * The end of a shelf row. Already Read carries the date it holds and a + * chevron, because it leads to the date pane; the others only mark the + * shelf the book is on. + */ + _renderShelfTrail(row) { + if (row.id === SHELF.ALREADY_READ) { + return html` + ${this.readDate ? html`${formatReadDate(this.readDate)}` : nothing} + + `; + } + return this.shelf === row.id ? html`` : nothing; + } + _renderStars() { const shown = this._hoverRating || this.rating || 0; // Once rated, the caption becomes an actionable "Clear rating" link. @@ -700,16 +851,32 @@ export class OlBookActions extends LitElement { `; } + /** + * Which row the recorded date is, so the pane shows the answer it already + * holds instead of reading as unanswered. Anything that is neither exactly + * today nor one of the offered years — a partial date included — belongs + * to "Other date". + */ + get _answeredBy() { + if (!this.readDate) return null; + const now = new Date(); + if (this.readDate === partialDate({ year: now.getFullYear(), month: now.getMonth() + 1, day: now.getDate() })) return 'today'; + if (quickYears(now).some(y => this.readDate === String(y))) return this.readDate; + return 'other'; + } + /** * Asked straight after the reader marks a book read. Two one-tap answers - * cover most cases; "Other date" swaps in the selects rather than taking a - * fourth pane, the same way the lists pane swaps in its create form. + * cover most cases; "Other date" discloses the selects underneath itself + * rather than replacing the rows or taking a fourth pane, so the two quick + * answers stay one tap away and the row you pressed stays on screen as the + * anchor. The track measures the pane, so the growth animates for free. * * A year on its own is a valid check-in, which is what makes "In 2026" * offerable at all. */ _renderCheckIn() { - const thisYear = new Date().getFullYear(); + const answered = this._answeredBy; return html`
${this.t('whenFinished')}
- ${this._pickingDate ? this._renderDateFields() : html` -
- - + ${quickYears().map(year => html` + - -
- `} + `)} + +
+ ${this._pickingDate ? this._renderDateFields() : nothing} `; } @@ -744,19 +935,28 @@ export class OlBookActions extends LitElement { const years = Array.from({ length: 121 }, (_, i) => thisYear - i); const days = month ? new Date(Number(year), Number(month), 0).getDate() : 31; return html` - + { if (e.key === 'Escape') { e.stopPropagation(); this._toggleDatePicker(); } }} + > +
- this._setDatePart('year', e.target.value)}> + + ${years.map(y => html``)} - this._setDatePart('month', e.target.value)}> + + ${MONTHS().map((name, i) => html``)} - this._setDatePart('day', e.target.value)}> + + ${Array.from({ length: days }, (_, i) => i + 1).map(d => html``)}
@@ -842,11 +1042,33 @@ export class OlBookActions extends LitElement { if (changed.has('_pane')) this._syncTrackHeight(); } + connectedCallback() { + super.connectedCallback(); + document.addEventListener('ol-list-created', this._onListCreatedElsewhere); + } + disconnectedCallback() { super.disconnectedCallback(); this._resizeObserver?.disconnect(); + document.removeEventListener('ol-list-created', this._onListCreatedElsewhere); } + /** + * A list created elsewhere on the page — a sibling popover or the legacy + * dropper — folded into this popover's pane so it stays honest without a + * refetch. Legacy creations also drop the shared cache: popovers that have + * not loaded yet must not resolve from a promise that predates the list. + */ + _onListCreatedElsewhere = (e) => { + if (e.target === this) return; + const { key, name, seedKey } = e.detail || {}; + if (!key) return; + if (e.target?.tagName !== 'OL-BOOK-ACTIONS') resetListsCache(); + if (this._lists && !(key in this._lists)) { + this._lists = { [key]: { listName: name, members: seedKey ? [seedKey] : [] }, ...this._lists }; + } + }; + /** Size the track to the active pane so the panel doesn't stretch to the taller one. */ _syncTrackHeight() { const pane = this.shadowRoot.querySelector(`.pane:nth-child(${this._paneIndex + 1})`); @@ -902,12 +1124,28 @@ export class OlBookActions extends LitElement { // ── Shelves ────────────────────────────────────────────── async _onShelfClick(shelfId) { + const previous = this.shelf; + // Already Read leads to the date pane — that is what its chevron says, + // and it is the only way to change a date once given. Coming off the + // shelf is the "Remove from shelf" row's job. + if (shelfId === SHELF.ALREADY_READ && previous === SHELF.ALREADY_READ) { + return this._openCheckIn(); + } + return this._postShelf(shelfId); + } + + /** Takes the book off whichever shelf it is on. Also what the main button does. */ + _removeFromShelf() { + if (this.shelf) return this._postShelf(this.shelf); + } + + /** Posting the current shelf toggles it off server-side; any other shelf moves the book. */ + async _postShelf(shelfId) { const previous = this.shelf; const removing = previous === shelfId; this.shelf = removing ? null : shelfId; this._busy = true; try { - // Posting the current shelf toggles it off server-side. await setShelf(this.book.key, shelfId, { editionKey: this.book.editionKey }); trackEvent('ReadingLog', removing ? 'RemoveFromShelf' : SHELF_EVENT[shelfId]); this._emitState(); @@ -952,16 +1190,25 @@ export class OlBookActions extends LitElement { async _openCheckIn() { this._pane = 'checkIn'; - this._pickingDate = false; - this._date = { year: '', month: '', day: '' }; + // A date the shortcuts cannot express would otherwise sit unseen + // behind a collapsed row, so the pane opens on it. Focus still lands + // on the first row: the reader is being shown their answer, not asked + // to retype it. + this._pickingDate = this._answeredBy === 'other'; + // Seeded from the date already given, so "Other date" opens on it + // rather than making the reader re-enter what they are amending. + const [year = '', month = '', day = ''] = (this.readDate || '').split('-'); + this._date = { year, month: month.replace(/^0/, ''), day: day.replace(/^0/, '') }; await this.updateComplete; this.shadowRoot.querySelector(`.pane:nth-child(${PANES.indexOf('checkIn') + 1}) .row`)?.focus({ preventScroll: true }); } - async _startPickingDate() { - this._pickingDate = true; + /** Focus follows the disclosure: into the selects, and back to the row on collapse. */ + async _toggleDatePicker() { + this._pickingDate = !this._pickingDate; await this.updateComplete; - this.shadowRoot.querySelector('.select.year')?.focus({ preventScroll: true }); + const target = this._pickingDate ? '.select.year' : '.date-toggle'; + this.shadowRoot.querySelector(target)?.focus({ preventScroll: true }); } /** Clearing a coarser part clears the finer ones, which the selects disable. */ @@ -977,8 +1224,8 @@ export class OlBookActions extends LitElement { return this._saveCheckIn({ year: now.getFullYear(), month: now.getMonth() + 1, day: now.getDate() }); } - _onThisYear() { - return this._saveCheckIn({ year: new Date().getFullYear() }); + _onYear(year) { + return this._saveCheckIn({ year }); } _onSaveDate(e) { @@ -996,8 +1243,13 @@ export class OlBookActions extends LitElement { if (this._dateBusy) return; this._dateBusy = true; try { - await setCheckIn(this.book.key, { ...date, editionKey: this.book.editionKey }); + const saved = await setCheckIn(this.book.key, { ...date, editionKey: this.book.editionKey, eventId: this.eventId }); trackEvent('CheckInPrompt', date.day ? 'SetDateDay' : date.month ? 'SetDateMonth' : 'SetDateYear'); + this.dispatchEvent(new CustomEvent('ol-book-check-in', { + bubbles: true, + composed: true, + detail: { key: this.book.key, date: partialDate(date), eventId: saved?.id ?? this.eventId ?? null }, + })); this._backToMain(); } catch (error) { this._fail(error); @@ -1082,11 +1334,16 @@ export class OlBookActions extends LitElement { try { const created = await createList(this.userKey, name, this._seedKey); trackEvent('Lists', 'CreateList'); - // Prepend so the new list is visible immediately; the shared cache - // is the same object, so sibling popovers see it too. + // Prepend so the new list is visible immediately. Sibling popovers + // and the legacy dropper hear about it through `ol-list-created`. this._lists = { [created.key]: { listName: name, members: [this._seedKey] }, ...this._lists }; _listsPromise = Promise.resolve(this._lists); this._creating = false; + this.dispatchEvent(new CustomEvent('ol-list-created', { + bubbles: true, + composed: true, + detail: { key: created.key, name, seedKey: this._seedKey }, + })); } catch (error) { this._fail(error); } finally { diff --git a/openlibrary/components/lit/OlOptionsPopover.js b/openlibrary/components/lit/OlOptionsPopover.js index 8c9a5e8e7b0..5f9dbb1e352 100644 --- a/openlibrary/components/lit/OlOptionsPopover.js +++ b/openlibrary/components/lit/OlOptionsPopover.js @@ -149,7 +149,7 @@ export class OlOptionsPopover extends FormAssociatedMixin(LitElement) { width: 16px; height: 16px; margin: 2px 0 0; - accent-color: var(--primary-blue); + accent-color: var(--color-primary); cursor: pointer; } diff --git a/openlibrary/components/lit/OlSelectPopover.js b/openlibrary/components/lit/OlSelectPopover.js index 061120776c3..4c6aaf00a43 100644 --- a/openlibrary/components/lit/OlSelectPopover.js +++ b/openlibrary/components/lit/OlSelectPopover.js @@ -239,7 +239,7 @@ export class OlSelectPopover extends FormAssociatedMixin(LitElement) { width: 16px; height: 16px; margin: 0; - accent-color: var(--primary-blue); + accent-color: var(--color-primary); cursor: pointer; } diff --git a/openlibrary/components/lit/OlShelfButton.js b/openlibrary/components/lit/OlShelfButton.js index 9edb39fc6d3..64a198caad9 100644 --- a/openlibrary/components/lit/OlShelfButton.js +++ b/openlibrary/components/lit/OlShelfButton.js @@ -49,6 +49,9 @@ const SHELF_LABEL = { * @prop {Number} shelf - Current shelf id (1–4), or null when on none * @prop {Number} rating - Current rating (1–5), or null. Passed through to the * popover and echoed on every state change + * @prop {String} readDate - Check-in date, whole or partial, shown on the + * popover's Already Read row. Applied by the surface, like shelf and rating + * @prop {Number} eventId - Id of that check-in, so editing the date amends it * @prop {String} userKey - "/people/" when signed in; empty sends the * visitor to log in instead of opening the popover * @prop {String} placement - ol-popover placement for the actions panel; @@ -59,6 +62,8 @@ const SHELF_LABEL = { * * @fires ol-book-state-change - The shelf or rating changed, optimistically or * rolled back. detail: { key, shelf, rating } + * @fires ol-book-check-in - Re-fired from the popover when a finish date is + * saved. detail: { key, date, eventId } */ export class OlShelfButton extends LitElement { static properties = { @@ -68,6 +73,8 @@ export class OlShelfButton extends LitElement { bookTitle: { type: String, attribute: 'book-title' }, shelf: { type: Number }, rating: { type: Number }, + readDate: { type: String, attribute: 'read-date' }, + eventId: { type: Number, attribute: 'event-id' }, userKey: { type: String, attribute: 'user-key' }, placement: { type: String }, labels: { type: Object }, @@ -88,17 +95,28 @@ export class OlShelfButton extends LitElement { /* ── Split variant ────────────────────────────────────────── */ + /* Same surface as ol-button: raised shadow plus the inset specular top edge. */ .split { display: flex; border: 1px solid var(--color-border-subtle); border-radius: var(--border-radius-button); overflow: hidden; background: var(--white); + --control-highlight-strength: 35%; + box-shadow: + var(--box-shadow-raised), + inset 0 1px 0 + color-mix( + in srgb, + var(--white) var(--control-highlight-strength), + var(--control-surface) + ); } .split--on { border-color: var(--color-control-selected-border); background: var(--color-control-selected-bg); + --control-surface: var(--color-control-selected-surface); } .main, @@ -113,7 +131,7 @@ export class OlShelfButton extends LitElement { color: var(--color-text); font-family: var(--font-family-button); font-size: var(--font-size-body-medium); - font-weight: 600; + line-height: var(--line-height-control); cursor: pointer; } @@ -148,9 +166,32 @@ export class OlShelfButton extends LitElement { color: var(--color-link); } - .main:hover, - .more:hover { - background: var(--color-hover-overlay); + /* Hover mirrors ol-button secondary/selected: the hovered half takes the + fill, and the outline darkens in step so the shape reads as one. */ + @media (hover: hover) and (pointer: fine) { + .main:hover, + .more:hover { + background: var(--color-control-hover); + } + + .split:has(.main:hover, .more:hover) { + border-color: var(--color-border-muted); + --control-surface: var(--color-control-hover); + } + + .split--on .main:hover, + .split--on .more:hover { + background: var(--color-control-selected-bg-hover); + } + + .split--on:has(.main:hover, .more:hover) { + border-color: var(--color-control-selected-border-hover); + --control-surface: var(--color-control-selected-surface-hover); + } + + .split--on:has(.main:hover, .more:hover) .more { + border-left-color: var(--color-control-selected-border-hover); + } } .main:focus-visible, @@ -265,6 +306,8 @@ export class OlShelfButton extends LitElement { .book=${{ key: this.workKey, title: this.bookTitle, editionKey: this.editionKey }} .shelf=${this.shelf} .rating=${this.rating} + .readDate=${this.readDate} + .eventId=${this.eventId} .labels=${this.labels} user-key=${this.userKey} placement=${ifDefined(this.placement)} diff --git a/openlibrary/components/lit/utils/books-api.js b/openlibrary/components/lit/utils/books-api.js index c8f0626b77a..238ace10a5c 100644 --- a/openlibrary/components/lit/utils/books-api.js +++ b/openlibrary/components/lit/utils/books-api.js @@ -74,14 +74,19 @@ export const EVENT = Object.freeze({ START: 1, UPDATE: 2, FINISH: 3 }); * POST /works/OL..W/check-ins — when the reader finished the book. * `month` and `day` are optional: a year alone, or a year and month, are both * valid check-ins, which is what lets the UI offer "in 2026". + * + * `eventId` edits that check-in in place. Without it the server records another + * one, which would count as a second book finished — so pass it whenever the + * reader is changing a date they already gave. */ -export function setCheckIn(workKey, { year, month = null, day = null, editionKey } = {}) { +export function setCheckIn(workKey, { year, month = null, day = null, editionKey, eventId = null } = {}) { return request(`/works/${olid(workKey)}/check-ins`, json({ event_type: EVENT.FINISH, year, month, day, edition_key: editionKey || null, + event_id: eventId || null, })); } diff --git a/openlibrary/core/bookshelves_events.py b/openlibrary/core/bookshelves_events.py index 50b34257531..22650c61baa 100644 --- a/openlibrary/core/bookshelves_events.py +++ b/openlibrary/core/bookshelves_events.py @@ -66,6 +66,24 @@ def get_latest_event_date(cls, username, work_id, event_type): results = list(oldb.query(query, vars=data)) return results[0] if results else None + @classmethod + def get_latest_event_dates_for_works(cls, username: str, work_ids: list[int], event_type: int) -> dict[int, dict]: + """The most recent event of `event_type` per work, for a batch of works. + + The single-work version costs a query each, which a page rendering + twenty search results cannot afford. + """ + if not work_ids: + return {} + oldb = db.get_db() + data = {"username": username, "work_ids": work_ids, "event_type": event_type} + query = ( + f"SELECT DISTINCT ON (work_id) work_id, id, event_date FROM {cls.TABLENAME}" + " WHERE username=$username AND work_id IN $work_ids AND event_type=$event_type" + " ORDER BY work_id, event_date DESC" + ) + return {row.work_id: row for row in oldb.query(query, vars=data)} + @classmethod def get_user_yearly_read_counts(cls, username: str) -> list[tuple[int, int]]: """Returns books read by year for a given user.""" diff --git a/openlibrary/i18n/messages.pot b/openlibrary/i18n/messages.pot index a4dd81bc4e6..7906d4b3892 100644 --- a/openlibrary/i18n/messages.pot +++ b/openlibrary/i18n/messages.pot @@ -718,7 +718,7 @@ msgstr "" msgid "Now" msgstr "" -#: admin/graphs.html admin/index.html my_books/check_ins/check_in_form.html my_books/check_ins/check_in_prompt.html trending.html +#: admin/graphs.html admin/index.html my_books/book_actions_i18n.html my_books/check_ins/check_in_form.html my_books/check_ins/check_in_prompt.html trending.html msgid "Today" msgstr "" @@ -751,13 +751,13 @@ msgid "Earliest trending data is from October 2017" msgstr "" #. Label for the reading log shelf for books the user plans to read in the future (bookshelf ID 1). Used as a button label and shelf name. -#: account/mybooks.html account/sidebar.html my_books/dropdown_content.html my_books/primary_action.html search/sort_options.html trending.html +#: account/mybooks.html account/sidebar.html my_books/book_actions_i18n.html my_books/dropdown_content.html my_books/primary_action.html search/sort_options.html trending.html msgid "Want to Read" msgstr "" #. Display name for the "currently-reading" reading log shelf used in page headings and breadcrumbs. #. Label for the reading log shelf for books the user is actively reading (bookshelf ID 2). Used as a button label and shelf name. -#: account/mybooks.html account/readinglog_shelf_name.html account/sidebar.html my_books/dropdown_content.html my_books/primary_action.html search/sort_options.html trending.html +#: account/mybooks.html account/readinglog_shelf_name.html account/sidebar.html my_books/book_actions_i18n.html my_books/dropdown_content.html my_books/primary_action.html search/sort_options.html trending.html msgid "Currently Reading" msgstr "" @@ -768,7 +768,7 @@ msgstr "" #. Display name for the "stopped-reading" reading log shelf used in page headings and breadcrumbs. #. Label for the reading log shelf for books the user stopped reading before finishing (bookshelf ID 4). Used as a button label and shelf name. #. Label for the reading log shelf for books the user stopped reading (bookshelf ID 4). Used as a button label and shelf name. -#: account/mybooks.html account/readinglog_shelf_name.html account/sidebar.html my_books/dropdown_content.html my_books/primary_action.html search/sort_options.html trending.html +#: account/mybooks.html account/readinglog_shelf_name.html account/sidebar.html my_books/book_actions_i18n.html my_books/dropdown_content.html my_books/primary_action.html search/sort_options.html trending.html msgid "Stopped Reading" msgstr "" @@ -1293,7 +1293,7 @@ msgstr "" #. Display name for the "already-read" reading log shelf used in page headings and breadcrumbs. #. Label for the reading log shelf for books the user has finished reading (bookshelf ID 3). Used as a button label and shelf name. -#: account/mybooks.html account/readinglog_shelf_name.html account/sidebar.html my_books/dropdown_content.html my_books/primary_action.html openlibrary/plugins/upstream/mybooks.py search/sort_options.html +#: account/mybooks.html account/readinglog_shelf_name.html account/sidebar.html my_books/book_actions_i18n.html my_books/dropdown_content.html my_books/primary_action.html openlibrary/plugins/upstream/mybooks.py search/sort_options.html msgid "Already Read" msgstr "" @@ -1395,7 +1395,7 @@ msgstr "" msgid "Yes! Please!" msgstr "" -#: EditButtons.html account/notifications.html account/privacy.html admin/block.html admin/spamwords.html covers/manage.html +#: EditButtons.html account/notifications.html account/privacy.html admin/block.html admin/spamwords.html covers/manage.html my_books/book_actions_i18n.html msgid "Save" msgstr "" @@ -1652,7 +1652,7 @@ msgstr "" msgid "See All" msgstr "" -#: account/sidebar.html type/list/edit.html type/series/edit.html +#: account/sidebar.html my_books/book_actions_i18n.html type/list/edit.html type/series/edit.html msgid "Create a list" msgstr "" @@ -3113,7 +3113,7 @@ msgstr "" msgid " by %(name)s" msgstr "" -#: books/daisy.html +#: books/daisy.html my_books/book_actions_i18n.html msgid "Back" msgstr "" @@ -5153,6 +5153,110 @@ msgstr "" msgid "comments" msgstr "" +#: my_books/book_actions_i18n.html +msgid "Remove from shelf" +msgstr "" + +#: my_books/book_actions_i18n.html +#, python-format +msgid "Actions for %(title)s" +msgstr "" + +#: my_books/book_actions_i18n.html +#, python-format +msgid "Save %(title)s to your reading log" +msgstr "" + +#: my_books/book_actions_i18n.html +#, python-format +msgid "%(title)s is on your reading log" +msgstr "" + +#: my_books/book_actions_i18n.html +#, python-format +msgid "More options for %(title)s" +msgstr "" + +#: my_books/book_actions_i18n.html +msgid "Rate this book" +msgstr "" + +#: my_books/book_actions_i18n.html +#, python-format +msgid "Rate %(rating)s of 5" +msgstr "" + +#: my_books/book_actions_i18n.html +msgid "Clear rating" +msgstr "" + +#: my_books/book_actions_i18n.html +msgid "Add to list" +msgstr "" + +#: my_books/book_actions_i18n.html +msgid "List name" +msgstr "" + +#: my_books/book_actions_i18n.html +msgid "Create" +msgstr "" + +#: my_books/book_actions_i18n.html +msgid "Filter lists…" +msgstr "" + +#: my_books/book_actions_i18n.html +msgid "You have no lists yet." +msgstr "" + +#: my_books/book_actions_i18n.html +msgid "No lists match." +msgstr "" + +#: my_books/book_actions_i18n.html +msgid "Loading lists…" +msgstr "" + +#: my_books/book_actions_i18n.html +#, python-format +msgid "%(count)s items" +msgstr "" + +#: my_books/book_actions_i18n.html +#, python-format +msgid "In %(count)s of your lists" +msgstr "" + +#: my_books/book_actions_i18n.html +msgid "Something went wrong. Please try again." +msgstr "" + +#: my_books/book_actions_i18n.html my_books/check_ins/check_in_prompt.html +msgid "When did you finish this book?" +msgstr "" + +#: my_books/book_actions_i18n.html +#, python-format +msgid "In %(year)s" +msgstr "" + +#: my_books/book_actions_i18n.html +msgid "Other date" +msgstr "" + +#: my_books/book_actions_i18n.html my_books/check_ins/check_in_form.html +msgid "Year" +msgstr "" + +#: my_books/book_actions_i18n.html my_books/check_ins/check_in_form.html +msgid "Month" +msgstr "" + +#: my_books/book_actions_i18n.html my_books/check_ins/check_in_form.html +msgid "Day" +msgstr "" + #: my_books/dropdown_content.html msgid "Remove From Shelf" msgstr "" @@ -5262,26 +5366,14 @@ msgstr "" msgid "Year:" msgstr "" -#: my_books/check_ins/check_in_form.html -msgid "Year" -msgstr "" - #: my_books/check_ins/check_in_form.html msgid "Month:" msgstr "" -#: my_books/check_ins/check_in_form.html -msgid "Month" -msgstr "" - #: my_books/check_ins/check_in_form.html msgid "Day:" msgstr "" -#: my_books/check_ins/check_in_form.html -msgid "Day" -msgstr "" - #: my_books/check_ins/check_in_form.html msgid "Delete Event" msgstr "" @@ -5299,10 +5391,6 @@ msgstr "" msgid "Read " msgstr "" -#: my_books/check_ins/check_in_prompt.html -msgid "When did you finish this book?" -msgstr "" - #: my_books/check_ins/check_in_prompt.html msgid "Other" msgstr "" diff --git a/openlibrary/macros/SearchResultsWork.html b/openlibrary/macros/SearchResultsWork.html index 38b32683f10..a4fb0b85dba 100644 --- a/openlibrary/macros/SearchResultsWork.html +++ b/openlibrary/macros/SearchResultsWork.html @@ -1,4 +1,4 @@ -$def with (doc, decorations=None, cta=True, availability=None, extra=None, attrs=None, rating=None, highlighting=None, show_librarian_extras=False, include_dropper=False, blur=False, footer=None, seq_index=None) +$def with (doc, decorations=None, cta=True, availability=None, extra=None, attrs=None, rating=None, highlighting=None, show_librarian_extras=False, include_dropper=False, blur=False, footer=None, seq_index=None, use_shelf_button=False, reading_state=None, shelf_labels=None, hide_rating=False) $code: max_rendered_authors = 9 @@ -268,7 +268,10 @@

$ edition_key = doc.get('edition_key') and doc.get('edition_key')[0] $if edition_key: $ edition_key = '/books/%s' % edition_key - $:render_template('my_books/dropper', doc, edition_key=edition_key, async_load=True) + $if use_shelf_button: + $:render_template('my_books/shelf_button', doc, edition_key=edition_key, reading_state=reading_state, labels=shelf_labels, hide_rating=hide_rating) + $else: + $:render_template('my_books/dropper', doc, edition_key=edition_key, async_load=True) $if rating: $:rating diff --git a/openlibrary/plugins/openlibrary/js/ile/utils/SelectionManager/SelectionManager.js b/openlibrary/plugins/openlibrary/js/ile/utils/SelectionManager/SelectionManager.js index 63bcf111251..621984fac9c 100644 --- a/openlibrary/plugins/openlibrary/js/ile/utils/SelectionManager/SelectionManager.js +++ b/openlibrary/plugins/openlibrary/js/ile/utils/SelectionManager/SelectionManager.js @@ -84,9 +84,11 @@ export default class SelectionManager { * @param {MouseEvent & { currentTarget: HTMLElement }} clickEvent */ processClick(clickEvent) { - // If there is text selection or the click is on a link that isn't a select handle, don't do anything + // If there is text selection or the click is on a link that isn't a select handle, don't do anything. + // `ol-shelf-button` is named explicitly: a click inside a shadow root + // retargets to the host, so the

- $ display_prompt = read_status == 3 -
- $_("When did you finish this book?") - - $ year = current_year() - $year - $_("Today") - $_("Other") - -
+ $if show_prompt: + $ display_prompt = read_status == 3 +
+ $_("When did you finish this book?") + + $ year = current_year() + $year + $_("Today") + $_("Other") + +
$if render_once("check-in-form-template"):