Refactor code for covers

[arkhi]

The goal of this PR is to reduce the complexity of the code for covers, and try to use a component-style approach because it is used in many places.

I might be going a bit heavy handed here, but that will help to actually understand the project better.

One of the goals is to remove classes like .is-large and use responsive images instead.

This PR will probably conflict with #971, but that’s fine when it will. :)

I will be using this script to generate screenshots with pageres along the way, to make sure (as much as possible) I am not breaking anything. I identified some URLs where a book cover would appear and I hope I am not missing too many. Let me know if that’s the case.

#! /bin/sh
#
# Usage: sh .pageres http://localhost:1333 [VALUE_OF_SESSION_COOKIE]

base=$1
cookie=$2

if [ -z "$base" ]; then
  echo "Please try again with a base URL. 👋"
  exit 0;
fi

  # /redraft-status/ID <=> post?status_type=TYPE&book=ID
if [ -n "$cookie" ]; then
  pageres \
    "$base/" \
    "$base/author/63" \
    "$base/book/361" \
    "$base/book/392" \
    "$base/book/990" \
    "$base/book/356/editions" \
    "$base/book/361/edit" \
    "$base/list" \
    "$base/list/1" \
    "$base/list/1/curate" \
    "$base/search/?q=kittens" \
    "$base/local" \
    "$base/federated" \
    "$base/discover" \
    "$base/get-started/books?query=3+secondes" \
    "$base/user/arkhi/" \
    "$base/user/arkhi/goal/2021" \
    "$base/user/arkhi/books/to-read" \
    "$base/post?status_type=comment&book=361" \
    320x568 \
    768x1024 \
    1024x768 \
    1216x960 \
    1920x1280 \
    --cookie="sessionid=$cookie" \
    --filename='logged-in-<%= date %>-<%= url %>-<%= size %><%= crop %>' \
    --format=jpg \
    --overwrite

  pageres \
    "$base/import/1" \
    320x9999 \
    768x9999 \
    1024x9999 \
    1216x9999 \
    1920x9999 \
    --crop \
    --cookie="sessionid=$cookie" \
    --filename='logged-in-<%= date %>-<%= url %>-<%= size %><%= crop %>' \
    --format=jpg \
    --overwrite
fi

pageres \
  "$base/" \
  "$base/local" \
  "$base/author/125" \
  "$base/book/361" \
  "$base/book/392" \
  "$base/book/990" \
  "$base/book/356/editions" \
  "$base/book/391/editions?page=2" \
  "$base/search/?q=kittens" \
  "$base/list" \
  "$base/list/1" \
  "$base/user/arkhi/" \
  "$base/user/arkhi/goal/2021" \
  "$base/user/arkhi/books/to-read" \
  320x568 \
  768x1024 \
  1024x768 \
  1216x960 \
  1920x1280 \
  --filename='logged-out-<%= date %>-<%= url %>-<%= size %><%= crop %>' \
  --format=jpg \
  --overwrite

[arkhi]

Thanks @mouse-reeve. I do like the fact we know what variables we’re working with in a given file, so having the context repeated in the file seems good to me. I do dislike the fact DTL does not allow for line-breaks and space management though, so I tend to not repeat the same variable in different places in the same file, hence the `{% with… %}` blocks. I also like to think about those blocks as a form of statement that says “Here, I want to work with that”.

[joachimesque]

In your list of pages to screenshoot, I'd add http://localhost:1333/discover: to my knowledge this is the only page where the book cover component is used with size="large". See https://lire.boitam.eu/discover, https://bookwyrm.social/discover

(in /bookwyrm/templates/book/book.html, the book cover snippet is called with size=large instead of size="large" so it doesn't work)

[mouse-reeve]

Yep, /discover is the only place "large" is used. /discover is the same as / when the user is logged out. Here's a deep cut, though, the standalone edit status page: http://localhost:1333/post?book=361&status_type=comment

[arkhi]

I'd add http://localhost:1333/discover

Thanks! I was playing with the logged out / which uses the same partial, but it won’t hurt to check this too. :)

(in /bookwyrm/templates/book/book.html, the book cover snippet is called with size=large instead of size="large" so it doesn't work)

Yep! I’ve fixed this while updating templates… I’ll see what broke when I get to this page and if I need to do anything.

Here's a deep cut, though, the standalone edit status page: http://localhost:1333/post?book=361&status_type=comment

Added, thanks!

[arkhi]

I think I updated all templates containing covers and they seem to be fine, from the screenshots I took and the tests I did.

I’ll have another round of reading after a night of sleep, then I’ll submit the Review.

[arkhi]

I think this PR is ready enough. I’m opening it to review; let’s see… :)

Better view the whitespace-free version.

Here is my goodreads_library_export.csv, in case you’d just want to run the script for screenshots with a fresh DB.

I tried to understand what the current rules about the book-cover were doing, then I tried to make it a more resilient component. It is not perfect; maybe the dimensional constraints could be handled by flex-direction and flex instead of fixed dimensions, but I think the changes work well enough for now.

• The container to be flexible enough by composing with other classes, to align its content inside it if necessary, or just adapts to its content based on its context.
• The column following a .is-cover column will regain access to an automatic flex-basis, allowing it to resize its main axis.
• Generally, I think the <a> around the covers should better be within the container. For example, adding a button in the container is impossible with the current DOM tree.

There were a couple of existing issues that also prompted this PR:

• Discover was sort of broken.
• Book was cropping covers that are not vertical.
• The content beside the cover was not always aligned if the covers are not all the same width.
• Spacings sometimes felt odd. (Some still do. :) )
• Columns were using a lot of extra padding.
• .content is used in many places for content that is not markdown-generated, causing unwanted side-effects.

Some Before / After

Author with one book

Author with multiple books

Editions with varying sized or missing covers

Lists with varying sized covers

Cropped content

Reading process

Clipped covers on Book

Funky Discover

---

Book editing

---

I took the opportunity to fix the horizontal scrolling for the getting started modal:

A few other untouched weirdnesses

Getting started on a tiny screen:

Import needs some table love on narrow screens:

What @joachimesque did on the User Books works. :)

---

There… That’s a lot. Thank you. :)

[arkhi]

@joachimesque: I implemented the list I mentioned in the past. I think it works fine but I can replace it with a table if need be.

[mouse-reeve]

This is a big improvement! My only qualm is with the display of book previews on reviews in mobile:

I'd prefer a large cover preview, but maybe the easiest fix here is to move the rate action and stars below the cover/snippet so that it doesn't constrain the width of the cover column?

[arkhi]

I'd prefer a large cover preview, but maybe the easiest fix here is to move the rate action and stars below the cover/snippet so that it doesn't constrain the width of the cover column?

I’d prefer that too. :)

I actually started doing that and, considering the increasing size of the PR, I went back to try to change what was necessary more than all I think I could improve. I sometimes got carried away, especially at the beginning.

I think it’s better if this specific layout can be solved in another smaller PR. Do you agree?

Also, an update on my last commit and a screenshot of the even more visible issue in French; I can only imagine the problem when German will come into play:

[kopischke]

I can only imagine the problem when German will come into play

From my experience doing both German and French localisation, total localised text length is mostly smaller in German than in French (German, a language devoid of most niceties, can be pretty terse; compare „Rauslehnen verboten“ with « Interdiction de se pencher »). The biggest issue are the bloodylongcompositewords German contains, which cause problems with reflow (you’d need automated hyphenation) and truncation (there are rules for correct truncation in German, but again, no automated support AFAIK).

[mouse-reeve]

@arkhi agreed! apologies for the merge conflicts, otherwise I'm happy to merge

[arkhi]

From my experience doing both German and French localisation, total localised text length is mostly smaller in German than in French (German, a language devoid of most niceties, can be pretty terse; compare „Rauslehnen verboten“ with « Interdiction de se pencher »). The biggest issue are the bloodylongcompositewords German contains, which cause problems with reflow (you’d need automated hyphenation) and truncation (there are rules for correct truncation in German, but again, no automated support AFAIK).

@kopischke: I had been seeing some of those bloodylongcompositewords issues during my (very quick) study of German and working for a German website, so I was taking this case into account more than basing my comment on any accurate knowledge of the language. I hope you didn’t get offended as mockery was not my intention; I’m mostly trying to think about the worst case scenarii if possible and German composite words were what popped in my mind.

Thanks for the feedback; it’s good to learn a bit more. :)

In our specific case, I don’t believe line truncation on buttons would really work (I’d go for hiding text overflow and ellipsis), but a rework of the interface could be a first step to avoid hiding any text at all.

[kopischke]

I hope you didn’t get offended as mockery was not my intention.

@arkhi No worries there; I very much appreciate the BookWyrm project’s awareness of these issues (the discussion on gender neutral French forms I found illuminating in how it mirrors many of our own – I was pretty much only aware of the point médian until then).

[arkhi]

@arkhi agreed! apologies for the merge conflicts, otherwise I'm happy to merge

No need to apologise. Part of the conflicts came from me formatting templates further than I should; my responsibility. :)

I’m very happy about the inclusion of inventaire.io in the mix and the simplified display of author[ (date)]!