Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bookshelf and cards refactoring #3037

Merged
merged 11 commits into from
Jun 25, 2024
Merged

Conversation

mikiher
Copy link
Contributor

@mikiher mikiher commented Jun 3, 2024

This refactoring addresses a number of issues I found while trying to add display of an optional subtitle line (which I left out at the moment):

  • The book card (and other cards) width and height are passed as props
    • This can (and does) cause incosistencies, where various card containers use different values. For example book cards currently have slightly different size on the home and library pages.
  • The height of the card does not reflect the actual card height. the bottom book details use absolute positioning outside the card area.
    • This requires complex layout computation that also happens outside the card, to make the card containers have the correct height to display both the card image and the details below.
  • sizeMultiplier is used many times in each of the cards.
    • Wherever some size attribute is set, it is multiplied by sizeMultiplier. This clutters the code quite a bit.
    • The sizeMultiplier is computed differently in each of the cards, although the result should be the same
  • bookCoverAspectRatio is passed as prop
    • This can also be computed in a single place.
  • There's is a separate itemSlider component for each card type.
    • The Vue templates and code in most component are mostly duplicates.
  • The bookshelf scaling of spacing and font-sizes is inconsistent
    • When the scaling changes (by increasing or decreasing the cover size), some of the spacing and font-sizes change, some don't

The following bookshelf fixes and refactoring were implemented (with the intention of leaving functionality as-is, except for bug fixes):

  • The height and width of bookshelf cards is now determined by default inside the card
    • The height of the card graphics (cover image or material icon) has a default.
    • All other properties are computed.
    • the card computed height now includes the bottom description if it has one.
      • one exception to this is the description in standard view, which needs to appear on top of the shelf divider
  • The shelf height is now determined by the computed height of the cards inside it
    • In lazyBookshelf, where the height needs to be determined in advance, one dummy card of the right type is mounted in order to get its width and height, and then removed.
    • This makes shelf height calculation straightforward and less error-prone.
  • All the itemSliders are now merged into a single component
    • The card component is dynamically determined by the shelf type.
    • Also fixed some small edge case slider scrolling issue, and made it not dependent on the card width
  • sizeMultiplier and bookCoverAspect ratio are now obtained from the store
  • The scaling and font sizes is now made consistent by changing them to relative em units
    • This is mostly done transparently by changing the default tailwind definitions
    • A version of the original tailwind spacing is still available (by adding r to the spacing number, e.g. right-4r)
    • At the bookshelf level, the font-size is set to sizeMultiplier * 1rem
      • this makes the tailwind classes inside the bookshelf reactive to the sizeMultplier (since they are now specified in em units)
      • this also makes sizeMultiplier multiplication redundant in most places (by changing rem units to em), making the code less cluttered.
  • Fixed some buggy booksPerFetch behavior
    • When cover size is set to smallest, lazyBookshelf doesn't get enough books to fill the first page, so something like this appears:
      Screenshot 2024-06-02 085102 This was due to booksPerFetch having static values. It was fixed to have a value depending on the the number of books per shelf and the number of shelfs per page.

I've tested this quite extensively in library, search, and home pages, in both stadard and detail view, while changing cover sizes back and forth. I believe all is working as intended now.

@mikiher mikiher marked this pull request as ready for review June 3, 2024 06:32
@mikiher
Copy link
Contributor Author

mikiher commented Jun 4, 2024

The last commit fixes #2774.

If you don't want this now, I can remove it - I just wanted to demonstrate how it's quite easy now to make changes to a card that increase its height, by just adding the required html to the card template, and without needing to modify height calculations anywhere - all layout modification to the bookshelf or other card containers happen automatically!

@mikiher
Copy link
Contributor Author

mikiher commented Jun 4, 2024

OK, please hold on with reviewing, I found some issues with tailwind font-size and line-height replacements.

… with em-based variants
@mikiher
Copy link
Contributor Author

mikiher commented Jun 23, 2024

So, an update (sorry it took quite a bit, I've been on vacation for most of the last couple of weeks): it looks like fully migrating to em-based spacing and font-sizing tailwind classes as a default has all sorts of side-effects on components unrelated to this refactoring (mostly due to know shrinking font issues). I tried fixing some of these but it just seems like too much changes all over the place.

I ended up reverting the the move to default em-based spacing and font-sizing. Instead:

  • I extended tailwind spacing to add em-based classes (by adding an e to the class name, e.g. px-2e as the em-based version of px-2).
  • I modifed refactored components to use the em-based tailwind versions for spacing, and explicit style for em-based font-sizing.

Now everything within the various bookshelf pages scales well when the cover size changes, and many things that didn't scale properly before (spacing, divider sizes font-size, material icon sizes, etc.) now scale correctly. In addition, the authors page can now also be scaled according to sizeMultiplier.

I think this is ready for review now.

@advplyr
Copy link
Owner

advplyr commented Jun 24, 2024

This is working great in my testing. I don't think we should include the show subtitles in this PR.
It doesn't make sense to show that setting on the regular bookshelf view. It is too tight on mobile for the library page. I think we should use a context menu dropdown if we add more checkboxes to the bookshelf toolbar. Showing series and genres there confused me at first and when it is empty it looks weird.

@mikiher
Copy link
Contributor Author

mikiher commented Jun 25, 2024

I don't think we should include the show subtitles in this PR.

Sure, I'll revert that, and rework it in a different PR based on your comments.

mikiher added 3 commits June 25, 2024 08:57
This reverts commit 3ef189e.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@advplyr
Copy link
Owner

advplyr commented Jun 25, 2024

Great, thanks!

@advplyr advplyr merged commit 0420350 into advplyr:master Jun 25, 2024
4 checks passed
@devnoname120
Copy link
Contributor

@mikiher Fantastic work, thanks a lot!

@mikiher mikiher deleted the bookshelf-refactor-2 branch July 12, 2024 18:20
@advplyr
Copy link
Owner

advplyr commented Aug 21, 2024

I just noticed that the LazyCollectionCard isn't updated for non-square covers anymore.

image

vs on the collection landing page
image

@mikiher
Copy link
Contributor Author

mikiher commented Aug 22, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants