Refactors and improves the My Books UI + architecture #8597

mekarpeles · 2023-12-07T05:53:42Z

Refactors mybooks.py routes into separate endpoints
Cleans up the main My Books Wrapper template
Makes /people/{username}/books visitable and sharable when readinglog set to public
Standardizes the page title using breadcrumb navigation dropper
Adds avatar image to mybooks breadcrumbs
Standardizes Edit --vanilla btn styling
Adds My Books Wrapper treatment to Lists and List
Shows "My" or patron's username in breadcrumb to clarify which page one is on
Standardizes chip buttons beneath header across pages
Moves "share" btn to its own spot in header

Known Bugs:

Tested (http://ol-dev1.us.archive.org:1337/)

Prepares for #739 done in https://github.com/internetarchive/openlibrary/tree/follows-and-mb-refactor

Technical

Testing

Screenshot

Stakeholders

codecov-commenter · 2023-12-07T19:59:11Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f6a69b6) 17.61% compared to head (1d93856) 17.61%.
Report is 16 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #8597   +/-   ##
=======================================
  Coverage   17.61%   17.61%           
=======================================
  Files          85       85           
  Lines        4456     4456           
  Branches      782      782           
=======================================
  Hits          785      785           
  Misses       3187     3187           
  Partials      484      484

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

- Refactors mybooks.py routes into separate endpoints - Cleans up the main My Books Wrapper template - Makes /people/{username}/books visitable and sharable when readinglog set to public - Standardizes the page title using breadcrumb navigation dropper - Adds avatar image to mybooks breadcrumbs - Standardizes Edit --vanilla btn styling - Adds My Books Wrapper treatment to Lists and List - Shows "My" or patron's username in breadcrumb to clarify which page one is on - Standardizes chip buttons beneath header across pages - Moves "share" btn to its own spot in header Known Bugs: - [X] Public "{username}'s books" breadcrumb breaks, shows just as "{username}'s" - [X] Breadcrumb dropdown unclickable, likely z-index error - [X] My Sponsorships broken - [X] URLs on My Books page carousels should reflect page owner, not logged in patron - [X] Observations breaks for public user (e.g. incognito /people/mekBot/books/observations) - [X] Private accounts breadcrumbs shouldn't show readinglog counts - [ ] Public List not rendering with my books sidebar - [ ] Share button does not preserve port Tested - [X] "My Books" works (as me), shows share, (stats, privacy & export chips) - [X] "My Books" loans, readinglogs, reviews, notes, stats all work - [X] Public "mekBot's Books" incognito works, notes + stats + loans all blocked - [X] Public "mekBot's" sponsorships and observations are blocked? - [X] "jachamp's Books" incognito successfully shows "private" + shows public lists + lists & list work - [X] Public account works for all 3 reading logs, NOT notes

openlibrary/plugins/openlibrary/lists.py

And robust to usernames with special characters

mekarpeles · 2023-12-08T01:20:17Z

openlibrary/plugins/openlibrary/lists.py

-        mb = MyBooksTemplate(username, 'lists')
-        template = self.render_template(path, show_header=False)
-        return mb.render(template=template, header_title=_("Lists"), page=mb.user)
+        if path.startswith("/people/"):


I wonder if we should simply break the path regex into different functions rather than trying to collapse them into one, just a suggestion.

Long term these should be two separate classes I reckon. They're pretty different in terms of function.

- top menu chips now wrap - my books stats permissions work on mobile, (chrome, safari) - deleted duplicate share buttons - fixed mobile alignment of share button

for more information, see https://pre-commit.ci

cdrini

Tested:

✅ non-people /lists page
✅ people /lists page
✅ Imports & exports
- Nitpick: dropper says "My Imports and exports", sidebar says "Imports & Exports Options"
✅ /account/loans
- Bug (pre-existing): "Loans" not highlighted in sidebar when on loans page
✅ stats works / is private
✅ sorting reading log works for myself and others
✅ Searching reading log works for myself and others
- Bug (pre-existing): A search with no matches confusingly shows the "You have no books on this shelf" screen"

openlibrary/plugins/upstream/mybooks.py

cdrini · 2023-12-08T00:40:54Z

openlibrary/plugins/upstream/mybooks.py

-        ratings=None,
-        year=None,
-    ):
+        if self.me and self.is_my_page and self.key in self.ALL_KEYS:


Suggested change

if self.me and self.is_my_page and self.key in self.ALL_KEYS:

if self.is_my_page and self.key in self.ALL_KEYS:

cdrini · 2023-12-11T15:39:35Z

openlibrary/plugins/openlibrary/lists.py

-        mb = MyBooksTemplate(username, 'lists')
-        template = self.render_template(path, show_header=False)
-        return mb.render(template=template, header_title=_("Lists"), page=mb.user)
+        if path.startswith("/people/"):


Long term these should be two separate classes I reckon. They're pretty different in terms of function.

cdrini · 2023-12-11T16:08:00Z

openlibrary/templates/account/mybooks.html

    $if owners_page:
+      <div>
+        <h1 class="details-title">My Stats</h1>


Suggested change

<h1 class="details-title">My Stats</h1>

<h1 class="details-title">$_('My Stats')</h1>

cdrini · 2023-12-11T16:09:19Z

openlibrary/templates/account/topmenu.html

@@ -0,0 +1,25 @@
+$def with (mb)
+
+$ readinglog = mb.key in ['currently-reading', 'already-read', 'want-to-read']


This would be better in the MyBooks class; some is_readinglog(key: str) -> bool

cdrini · 2023-12-11T16:10:24Z

openlibrary/templates/account/view.html

This should be name mybooks_shell.html. view.html makes me think it's /people/ScarTissue , the profile page, since {type}/view.html correspond to infogamy types

openlibrary/templates/books/breadcrumb_select.html

cdrini · 2023-12-11T16:16:49Z

static/css/components/buttonCta.less

@@ -75,24 +75,27 @@ a.cta-btn {
    border: 2px solid @link-blue;
    background: lighten(desaturate(@link-blue, 56%), 67%);
  }
-  &--vanilla {
+  &--vanilla, &--vanilla:visited, &--vanilla:link {


Why are we doing this? I think this is frowned upon for accessibility.

cdrini

Lgtm! Small suggestions left. Merge at your discretion

jimchamp · 2023-12-12T22:06:03Z

Sidebar is broken on account settings page:

https://staging.openlibrary.org/account?debug=true

Suspect this is due to counts being None.

mekarpeles force-pushed the refactor/mybooks branch 6 times, most recently from fea1dfc to 8dfd08f Compare December 7, 2023 07:54

mekarpeles marked this pull request as ready for review December 7, 2023 08:04

mekarpeles force-pushed the refactor/mybooks branch from 8dfd08f to 13b545f Compare December 7, 2023 19:57

mekarpeles force-pushed the refactor/mybooks branch from 13b545f to 75f38c5 Compare December 7, 2023 20:41

mekarpeles added the On testing.openlibrary.org This PR has been deployed to testing.openlibrary.org for testing label Dec 7, 2023

cdrini reviewed Dec 7, 2023

View reviewed changes

openlibrary/plugins/openlibrary/lists.py Outdated Show resolved Hide resolved

openlibrary/plugins/openlibrary/lists.py Outdated Show resolved Hide resolved

cdrini force-pushed the refactor/mybooks branch from ff1044d to 3cd6ed6 Compare December 7, 2023 23:54

Fix non-people /lists endpoints

7bbf270

cdrini force-pushed the refactor/mybooks branch from 3cd6ed6 to 7bbf270 Compare December 7, 2023 23:55

Make mybooks path regexes consistent

1f0b0b6

And robust to usernames with special characters

mekarpeles commented Dec 8, 2023

View reviewed changes

mekarpeles and others added 2 commits December 8, 2023 05:16

Code review and QA fixes

5582aa1

- top menu chips now wrap - my books stats permissions work on mobile, (chrome, safari) - deleted duplicate share buttons - fixed mobile alignment of share button

[pre-commit.ci] auto fixes from pre-commit.com hooks

feaf1d2

for more information, see https://pre-commit.ci

mekarpeles assigned cdrini Dec 8, 2023

mekarpeles mentioned this pull request Dec 9, 2023

Adds follow (pubsub) feature #8607

Merged

3 tasks

cdrini reviewed Dec 11, 2023

View reviewed changes

cdrini added 2 commits December 11, 2023 16:18

Dissolve some MyBooks methods

2a751bc

Remove prefix from mybooks page

1d93856

cdrini approved these changes Dec 11, 2023

View reviewed changes

mekarpeles merged commit b121d32 into master Dec 11, 2023
4 checks passed

mekarpeles deleted the refactor/mybooks branch December 11, 2023 17:36

jimchamp mentioned this pull request Dec 12, 2023

Sidebar missing from settings page #8620

Closed

mekarpeles mentioned this pull request Dec 14, 2023

My Books visual fixes #8629

Merged

jimchamp removed the On testing.openlibrary.org This PR has been deployed to testing.openlibrary.org for testing label Apr 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactors and improves the My Books UI + architecture #8597

Refactors and improves the My Books UI + architecture #8597

mekarpeles commented Dec 7, 2023 •

edited

Loading

codecov-commenter commented Dec 7, 2023 •

edited

Loading

mekarpeles Dec 8, 2023

cdrini Dec 11, 2023

cdrini left a comment

cdrini Dec 8, 2023

cdrini Dec 11, 2023

cdrini Dec 11, 2023

cdrini Dec 11, 2023

cdrini Dec 11, 2023 •

edited

Loading

cdrini Dec 11, 2023

cdrini left a comment

jimchamp commented Dec 12, 2023

	if self.me and self.is_my_page and self.key in self.ALL_KEYS:
	if self.is_my_page and self.key in self.ALL_KEYS:

	<h1 class="details-title">My Stats</h1>
	<h1 class="details-title">$_('My Stats')</h1>

		@@ -0,0 +1,25 @@
		$def with (mb)

		$ readinglog = mb.key in ['currently-reading', 'already-read', 'want-to-read']

Refactors and improves the My Books UI + architecture #8597

Refactors and improves the My Books UI + architecture #8597

Conversation

mekarpeles commented Dec 7, 2023 • edited Loading

Known Bugs:

Tested (http://ol-dev1.us.archive.org:1337/)

Technical

Testing

Screenshot

Stakeholders

codecov-commenter commented Dec 7, 2023 • edited Loading

Codecov Report

mekarpeles Dec 8, 2023

Choose a reason for hiding this comment

cdrini Dec 11, 2023

Choose a reason for hiding this comment

cdrini left a comment

Choose a reason for hiding this comment

cdrini Dec 8, 2023

Choose a reason for hiding this comment

cdrini Dec 11, 2023

Choose a reason for hiding this comment

cdrini Dec 11, 2023

Choose a reason for hiding this comment

cdrini Dec 11, 2023

Choose a reason for hiding this comment

cdrini Dec 11, 2023 • edited Loading

Choose a reason for hiding this comment

cdrini Dec 11, 2023

Choose a reason for hiding this comment

cdrini left a comment

Choose a reason for hiding this comment

jimchamp commented Dec 12, 2023

mekarpeles commented Dec 7, 2023 •

edited

Loading

codecov-commenter commented Dec 7, 2023 •

edited

Loading

cdrini Dec 11, 2023 •

edited

Loading