Canonical Books Page (Merging works & Editions UI) #3553

mekarpeles · 2020-07-05T06:47:10Z

Closes #684 🎉
Closes #3556 🎉

Change Log

Adds sticky tab bar to books page
Show more / less collapse button
Books page chooses "best" (previewable) edition by default, shows selected edition in editions table
Search result shows cover consistent w/ available edition, sends to (3)
Adds editions table to the books page
Adds filter/search to editions table + ability to change # of results in table w/o reloading
Adds checkbox to List dropdown to control whether to add work v. edition to list
Adds "preview also available in" for various applicable languages
Fun / useless bookcover hover animation
Consolidates calls to action within sidebar + moves sidebar to the left, below bookcover

Technical

Merges the UI for /works and /books.
URLs are not touched
Lists are loaded multiple times on page (to enable showing edition + work results & separating locations of lists management from carousel/showcase)

Code Review Checklist

Is everything i18n?
no inline styles
js in plugins/openlibrary/js/ where possible (except for templates/lists/widget.html)

Known issues

BLOCKER: search results page items w/ no ocaid produce broken pages, e.g. "three body problem" ->
Read More link shouldn't display (for short descriptions / subject lists)
Styling on the subjects and pagination css affecting other pages (e.g. authors page)
https://dev.openlibrary.org/works/OL17335914W/The_Three-Body_Problem?edition=
Improve search results: include overview of available previews & languages #3556 Search result pages showing IA covers -- half fixed (when possible, we use the openlibrary_edition olid returned by the archive.org ES availability API. If the API has no olid, we fallback to archive.org (for now)
BWB buy-line too large for container (non-blocker)

Punting:

Odd editions line flex formatting Canonical Books Page (Merging works & Editions UI) #3553 (comment)
Certain /edit fields contain html which breaks Book Page design (e.g. italics) #3554 if missing authors, styling breaks on book page -- on issue tracker

Testing

For each search result, book cover same as selected ocaid in the work title / borrow btn
Clicking on a search result cover goes to the works page w/ correct ocaid selected
Check lists behave correctly, both for work + edition
LoanStatus: Borrowing, previews, and waitlists should work on books page
Changing editions should (currently) go to a /books URL

Screenshot

Stakeholders

@bfalling @JeffKaplan @tabshaikh @cdrini @seabelis et al

static/css/components/work.less

openlibrary/templates/type/work/editions_datatable.html

openlibrary/plugins/openlibrary/js/readmore.js

openlibrary/macros/ReadMore.html

openlibrary/macros/StarRatingsStats.html

openlibrary/macros/StarRatings.html

openlibrary/macros/SubjectTags.html

openlibrary/templates/type/edition/view.html

tabshaikh

Went through all the files and LGTM

openlibrary/plugins/openlibrary/js/readmore.js

openlibrary/macros/SubjectTags.html

cdrini

Reviewed the code; mostly some i18n things/invalid css/etc. Also, was there a bad rebase somewhere? A lot of these changes are about adding that author search bar, which I though was merged separately.

openlibrary/macros/EditionNavBar.html

cdrini · 2020-07-06T22:00:59Z

openlibrary/macros/SearchResultsWork.html

+ $_('in') <a title="$(', '.join(doc.languages))">$(len(doc.languages)) $_('languages')</a>
+ </span>
+ $if doc.get('ia'):
+ &mdash; $(len(doc.get('ia'))) $_('previewable')


i18n number in the string

Suggested change

— $(len(doc.get('ia'))) $_('previewable')

— $_('%s previewable' % len(doc.get('ia')))

Don't use %; use %d.

openlibrary/macros/SearchResultsWork.html

openlibrary/templates/type/edition/view.html

static/css/components/work.less

cdrini · 2020-07-06T22:59:09Z

Small style fixes:

This errors because it has no editions: https://dev.openlibrary.org/works/OL15331214W/Fellowship_of_the_Ring
Blue drop shadow around covers in search results looks odd. It also looks add as the image is loading, because it just displays as a horizontal line. (Also looks odd because it's clipped off).
Add dotted underline to "19 languages" in search results
This looks odd:

https://dev.openlibrary.org/works/OL15060203W/Tacos?edition=

See: #3553 Screenshot: https://user-images.githubusercontent.com/978325/86527110-9eda7700-be50-11ea-84ea-7b0651a77f79.png Change Log: - Adds sticky tab bar to books page - Show more / less collapse button - Books page chooses "best" (previewable) edition by default, shows selected edition in editions table - Search result shows cover consistent w/ available edition, sends to (3) - Adds editions table to the books page - Adds filter/search to editions table + ability to change # of results in table w/o reloading - Adds checkbox to List dropdown to control whether to add work v. edition to list - Adds "preview also available in" for various applicable languages - Fun / useless bookcover hover animation - Consolidates calls to action within sidebar + moves sidebar to the left, below bookcover Technical: - Merges the UI for /works and /books. - URLs are not touched - Lists are loaded multiple times on page (to enable showing edition + work results & separating locations of lists management from carousel/showcase) Co-authored-by: tabshaikh <[email protected]> Co-authored-by: Drini Cami <[email protected]> Co-authored-by: Michael E. Karpeles (mek) <[email protected]>

mekarpeles

Batching PR suggestions

static/css/components/work.less

openlibrary/macros/EditionNavBar.html

mekarpeles · 2020-07-07T00:14:07Z

openlibrary/macros/SearchResultsWork.html

+ $_('in') <a title="$(', '.join(doc.languages))">$(len(doc.languages)) $_('languages')</a>
+ </span>
+ $if doc.get('ia'):
+ &mdash; $(len(doc.get('ia'))) $_('previewable')


Suggested change

— $(len(doc.get('ia'))) $_('previewable')

— $_('%s previewable' % len(doc.get('ia')))

openlibrary/macros/SubjectTags.html

openlibrary/macros/databarView.html

openlibrary/plugins/openlibrary/js/editions-table/index.js

openlibrary/plugins/upstream/utils.py

openlibrary/templates/lists/widget.html

openlibrary/templates/type/edition/book_title.html

mekarpeles · 2020-07-07T05:00:24Z

#3553 (comment)
Fixes book pages w/ no edition available, fixed blue shadow on SERP + added dotted underline under search page "languages"

See: #3553 Screenshot: https://user-images.githubusercontent.com/978325/86527110-9eda7700-be50-11ea-84ea-7b0651a77f79.png Change Log: - Adds sticky tab bar to books page - Show more / less collapse button - Books page chooses "best" (previewable) edition by default, shows selected edition in editions table - Search result shows cover consistent w/ available edition, sends to (3) - Adds editions table to the books page - Adds filter/search to editions table + ability to change # of results in table w/o reloading - Adds checkbox to List dropdown to control whether to add work v. edition to list - Adds "preview also available in" for various applicable languages - Fun / useless bookcover hover animation - Consolidates calls to action within sidebar + moves sidebar to the left, below bookcover Technical: - Merges the UI for /works and /books. - URLs are not touched - Lists are loaded multiple times on page (to enable showing edition + work results & separating locations of lists management from carousel/showcase) Co-authored-by: tabshaikh <[email protected]> Co-authored-by: Drini Cami <[email protected]> Co-authored-by: Michael E. Karpeles (mek) <[email protected]>

mekarpeles · 2020-07-07T06:38:36Z

Thank you both!

I think I've addressed all of the blocking feedback and even most of the suggestions on this issue. At this point, I think we're pretty close. Not thrilled about the PR being having been squashed but I think the description of the commit and the scope is pretty thorough. A lot of the intermediary work was also scratch (and given what we had I think I prefer it being squashed, especially since the major refactors -- the Lists -- were done in a separate branch).

@tabshaikh, so we're ready for tomorrow's deploy, if you're comfortable, can you please do a final pass, making sure we didn't miss any of @cdrini's feedback and that there are no blockers? When you feel confident please merge and we can discuss during ABC to see if any followups are necessary to the PR before a deploy goes out.

There are 2 known issues remaining; the first is a formatting issue which I agree doesn't look amazing, but I'd prefer tackling this as a followup once the main branch is landed. The other is an edge case re: books w/ missing authors which has an issue open for it.

tabshaikh · 2020-07-07T12:56:22Z

As @mekarpeles pointed out there are 2 known issues remaining; the first is a formatting issue which I agree doesn't look amazing, but I'd prefer tackling this as a followup once the main branch is landed. The other is an edge case re: books w/ missing authors which has an issue open for it.
Also, we would like the sticky tab to change upon which content is in view.

Also Clear My Rating should be centered

Other than that LGTM :) merging

(Note: The history is funny because everything was squashed initially, and then reverted to be added back unsquashed) Change Log: - Adds sticky tab bar to the books page - Show more / less collapse button - Books page chooses "best" (previewable) edition by default, shows selected edition in editions table - Search result shows cover consistent w/ available edition, sends to (3) - Adds editions table to the books page - Adds filter/search to editions table + ability to change # of results in table w/o reloading - Adds a checkbox to List dropdown to control whether to add work v. edition to list - Adds "preview also available in" for various applicable languages - Fun / useless book cover hover animation - Consolidates calls to action within sidebar + moves sidebar to the left, below the book cover Technical: - Merges the UI for /works and /books. - URLs are not touched - Lists are loaded multiple times on page (to enable showing edition + work results & separating locations of lists management from carousel/showcase) Co-authored-by: tabshaikh <[email protected]> Co-authored-by: Michael E. Karpeles (mek) <[email protected]>

mekarpeles force-pushed the local/canonical-page-rebased branch from d1fcb3f to 63f7c74 Compare July 5, 2020 11:09

mekarpeles commented Jul 5, 2020

View reviewed changes

static/css/components/work.less Outdated Show resolved Hide resolved

mekarpeles commented Jul 5, 2020

View reviewed changes

openlibrary/templates/type/work/editions_datatable.html Outdated Show resolved Hide resolved

mekarpeles commented Jul 5, 2020

View reviewed changes

openlibrary/plugins/openlibrary/js/readmore.js Outdated Show resolved Hide resolved

mekarpeles mentioned this pull request Jul 5, 2020

Certain /edit fields contain html which breaks Book Page design (e.g. italics) #3554

Closed

mekarpeles force-pushed the local/canonical-page-rebased branch from 9067bea to 00e63df Compare July 6, 2020 07:21