Create My Books Dropper #8019

jimchamp · 2023-06-29T18:30:11Z

Closes #6531
Closes #6897
Closes #6992
Closes #8009

Creates reusable dropper component, and refactors code such that the My Books dropper uses the new generic dropper.

Issue where works cannot added to / removed from a list has been fixed as well.

Technical

The Generic Dropper Component

The generic dropper component comprised of a primary button and dropdown content. The template for the dropper takes a rendered HTML string for each of these. The template also takes a additional classes as a string, enabling the dropper to be styled as needed.

Visibility of the dropdown content can be changed by clicking the "dropclick" arrow. If a patron clicks outside of the dropper component when the dropdown is visible, it will be hidden. This functionality is business as usual (BAU). What is new is that dropdown visibility can now be changed with 'toggle-dropperandclose-dropperevents. Using these new event is as easy as passing a reference to a child element of a dropper to thefireDropperToggleEventandfireDropperCloseEvent` functions, which dispatch the appropriate event from the given element to the parent dropper.

The new template for generic droppers is /openlibrary/templates/lib/dropper.html. All BAU dropper functionality will automatically be added to components created using that template.

The My Books Dropper

The My Books Dropper combines reading log shelf, list, and last read date management in a single component. Three new templates have been created for this component:

/openlibrary/templates/my_books/dropper.html: Takes a page (work, edition, author, etc.) and renders a new My Books Dropper
/openlibrary/templates/my_books/dropdown_content.html: Renders reading log buttons (if applicable), dropdown list affordances, and the "Create a new list" modal content.
/openlibrary/templates/my_books/primary_action.html: Renders the primary reading log button, or "Add to list" CTA button (depending on context).

New JS classes have been created in order to better encapsulate different "My Books" actions:

ReadingLogForms handles everything related to our reading log bookshelves.
ReadDateComponents contains code that toggles visibility of our "Last read date" components.
ReadingLists handles all list management functionality.

Additionally, a new MyBooksDropper class has been created, which creates instances of the aforementioned classes (if possible), and asynchronously loads the patrons lists. Creating a new MyBooksDropper object will hydrate all parts of the dropper that the object represents.

The following diagram illustrates which class affects what part of the My Books dropper:

Note that the ReadingList class also provides the "Remove from list" functionality for showcase items:

General

The My Books droppers are feature flagged. Legacy "lists/widget" droppers will be used unless features.my_books_dropper is enabled in the openlibrary.yml. To quickly check if the new droppers are enabled, you can search the page's HTML source for the class dropper-wrapper, which is unique to My Books droppers.

New functions have been added to ListService.js which make /lists POST requests with fetch. The legacy, callback-based, approach was largely untestable.

Limitations

Lists can't be fully managed from search result pages currently (My Books reading log pages included). Items can be added to a list in this context, but not removed. This is due to the "Active Lists" showcase, which appears beneath the dropper on book pages, not being present in search results (too much vertical space needed).

Testing

When logged out:

Clicking the dropper should not open it.
Clicking the dropper should redirect to the login page.
Droppers styling should not change.

When logged in, on a book page:

All BAU dropper functionality on book pages works as expected.
Clicking the "Use this work" checkbox should update the view appropriately.

When logged in, on a search result page:

Books can be added to or removed from a reading log shelf (this is BAU).
Works can be added to lists.
No z-index issues observed while droppers are open.
All list loading indicators are replaced. If the indicator in the second dropper has been replaced, they all should have been.
Ensure that the correct item is being added to a list.

Screenshot

Next steps

We may want to avoid using the new My Books droppers until lists can be fully managed from search results pages. This will require list removal from inside of the dropdown content. Until that time, we could enable the droppers in testing in order to get feedback from folks.

We should also make sure that read date, list, and reading log data used to create these droppers is fetched in a performant manner (maybe in bulk and from a cache?).

After the feature is rolled out, we could consider the following improvements:

Create a dedicated template for the list showcases (found in the "Lists" section of the books page).
Remove unused legacy code (ReadingLogDropper.html, etc.).
Reevaluate the need for the lists/widget template.
Remove functions unused functions from ListService.js

Stakeholders

codecov-commenter · 2023-07-04T00:43:41Z

Codecov Report

Merging #8019 (78c0426) into master (c71ee55) will decrease coverage by 0.47%.
Report is 55 commits behind head on master.
The diff coverage is 15.15%.

@@            Coverage Diff             @@
##           master    #8019      +/-   ##
==========================================
- Coverage   17.14%   16.67%   -0.47%     
==========================================
  Files          71       83      +12     
  Lines        3750     4419     +669     
  Branches      647      757     +110     
==========================================
+ Hits          643      737      +94     
- Misses       2695     3199     +504     
- Partials      412      483      +71

Files Changed	Coverage Δ
openlibrary/plugins/openlibrary/js/index.js	`0.00% <0.00%> (ø)`
...ibrary/plugins/openlibrary/js/lists/ListService.js	`0.00% <0.00%> (ø)`
openlibrary/plugins/openlibrary/js/lists/index.js	`6.20% <0.00%> (ø)`
.../plugins/openlibrary/js/my-books/MyBooksDropper.js	`0.00% <0.00%> (ø)`
...y/js/my-books/MyBooksDropper/ReadDateComponents.js	`0.00% <0.00%> (ø)`
...library/js/my-books/MyBooksDropper/ReadingLists.js	`0.00% <0.00%> (ø)`
...rary/js/my-books/MyBooksDropper/ReadingLogForms.js	`0.00% <0.00%> (ø)`
...enlibrary/plugins/openlibrary/js/my-books/index.js	`0.00% <0.00%> (ø)`
...ary/plugins/openlibrary/js/my-books/store/index.js	`20.00% <20.00%> (ø)`
.../plugins/openlibrary/js/my-books/CreateListForm.js	`25.00% <25.00%> (ø)`
... and 4 more

... and 8 files with indirect coverage changes

openlibrary/macros/databarWork.html

mekarpeles · 2023-07-13T20:21:28Z

openlibrary/plugins/openlibrary/js/lists/ListService.js

+ * @param {object} data Object containing the new list's name, description, and seeds.
+ * @returns {Promise<Response>} The results of the POST request
+ */
+export async function createList(userKey, data) {


What happens if there is a request error/failure?

Nothing now, but maybe we trigger a toast message?

Quick note: any errors are meant to be handled by the caller of this function. Whatever we decide to do on list creation failure will be included inside of a catch() call, here:

openlibrary/openlibrary/plugins/openlibrary/js/my-books/ReadingLists.js

Lines 379 to 383 in 965c75f

await createList(this.userKey, data)

.then(response => response.json())

.then((data) => {

this.onListCreationSuccess(data['key'], listTitle)

})

mekarpeles · 2023-07-13T20:27:13Z

Q: Is it possible for us to use a param or a separate endpoint (e.g addtolist.json v. addtolist) one which returns json upon POST and the other (think of the case where js is disabled on client) the result redirects patron back to (book) page.

The problem we'd be trying to solve is making the Want to Read button work more naturally in the case where js is disabled (so it doesn't look like a bug / return json)

mekarpeles · 2023-07-13T20:28:36Z

openlibrary/plugins/openlibrary/lists.py

@@ -62,8 +62,17 @@ def get_seed_info(doc):

 @public
 def get_list_data(list, seed, include_cover_url=True):
+    list_items = []
+    for s in list.get_seeds():


@mekarpeles to check whether any caching here.

@jimchamp: may have performance issues if list has many seeds?

Noting that this section isn't properly feature-flagged. list_items should only be populated and added to list data d if my_books_dropper feature is enabled.

@jimchamp: may have performance issues if list has many seeds?

Yes, on book pages. This is only called once on search results pages, but several times in book pages. The public get_user_lists function also calls this while iterating over a list. Cleaning up the lists widget template a bit should help.

openlibrary/templates/lists/widget.html

jimchamp · 2023-07-18T20:45:30Z

Checkmarks are now used inside of the droppers to indicate list membership. The active list showcase, previously located below the dropper on some pages, has been removed completely.

The dropper as found on an author page.

jimchamp · 2023-07-18T21:00:58Z

bundlesize.config.json

@@ -78,11 +78,11 @@
    },
    {
      "path": "static/build/page-admin.css",
-      "maxSize": "25KB"
+      "maxSize": "26KB"


I'm surprised that this size needed to be adjusted. I wonder if our Less references need investigation.

jimchamp · 2023-07-19T15:47:09Z

openlibrary/templates/lib/dropper.html

@@ -0,0 +1,17 @@
+$def with(primary_button, dropdown_content, additional_classes='')


This needs comments. Would be good to know that passing generic-dropper--disabled as an additional class will disable the dropper for logged out patrons.

openlibrary/templates/lists/dropper_lists.html

jimchamp · 2023-07-27T01:59:47Z

Important Note: Feature flag should be reinstated. If we're seeing a severe performance hit on search pages after this is released, we'll want to be able to quickly revert to the old droppers.

cdrini · 2023-08-08T20:09:10Z

openlibrary/templates/lists/list_overview.html

+$ data_list_key = 'data-list-key=%s' % list.key if actionable else ''
+<li $actionable $data_list_key>


Hey @jimchamp do you mind if I commit this change? I think that should let both our PRs co-exist on testing (mine is #8168 )

Suggested change

$ data_list_key = 'data-list-key=%s' % list.key if actionable else ''

<li $actionable $data_list_key>

<li $actionable $:cond(actionable, 'data-list-key=%s' % list.key)>

Messaged you on slack and you preferred to remove it from testing. Removed it 👍 Accept this suggestion at your discretion 😋

- These are meant to be overridden by Dropper subclasses, if needed. - `onDisabledClick` is called whenever a disable dropper is clicked - `onOpen` is called when the dropper is opened - `onClose` is called when the dropper is closed - My Books dropper `toggleDropper` override has been replaced with an `onOpen` override.

jimchamp · 2023-09-16T00:54:15Z

Bugs fixed:

Author page rendering error
Clicking dropclick arrow while unauthenticated redirects to login page

Subclasses of Dropper no longer need to override toggleDropper() or closeDropper() to add additional functionality, as new on* event functions have been created:

onClose() is called when a dropper has been closed
onOpen() is called when a dropper has been opened
onDisabledClick() is called when the arrow of a disabled dropper has been clicked

The intention is that the new on* functions be overridden as needed.

New changes have not been deployed to LLEs.

mekarpeles

LGTM, there were 4 considerations outstanding last time we discussed:

Clicking dropdown arrow when logged out should redirect to login page
Errors on Authors page
Book covers not loading -- similar problems on production (at least for initial load)
Create an issue for ability to see selected lists in dropper even when minimized/closed – volunteer wanted (limit to 3, scrollable)

openlibrary/plugins/openlibrary/js/my-books/index.js

Co-authored-by: Mek <michael.karpeles@gmail.com>

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

jimchamp force-pushed the refactor/generic-dropper branch 5 times, most recently from ef80d92 to 5448579 Compare July 4, 2023 00:39

jimchamp marked this pull request as ready for review July 4, 2023 00:42

mekarpeles self-assigned this Jul 10, 2023

mekarpeles added Priority: 1 Do this week, receiving emails, time sensitive, . [managed] Needs: Special Deploy This PR will need a non-standard deploy to production labels Jul 10, 2023

mekarpeles reviewed Jul 13, 2023

View reviewed changes

openlibrary/macros/databarWork.html Outdated Show resolved Hide resolved

mekarpeles reviewed Jul 13, 2023

View reviewed changes

openlibrary/templates/lists/widget.html Outdated Show resolved Hide resolved

jimchamp force-pushed the refactor/generic-dropper branch from 7078bf5 to fdc7e30 Compare July 18, 2023 20:28

jimchamp commented Jul 18, 2023

View reviewed changes

jimchamp force-pushed the refactor/generic-dropper branch from f1f55f3 to ec70452 Compare July 19, 2023 15:34

jimchamp commented Jul 19, 2023

View reviewed changes

openlibrary/templates/lists/dropper_lists.html Outdated Show resolved Hide resolved

jimchamp added the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label Jul 26, 2023

jimchamp added the On testing.openlibrary.org This PR has been deployed to testing.openlibrary.org for testing label Jul 27, 2023

jimchamp force-pushed the refactor/generic-dropper branch 3 times, most recently from 456f6b5 to 946bc22 Compare August 7, 2023 15:31

cdrini reviewed Aug 8, 2023

View reviewed changes

cdrini removed the On testing.openlibrary.org This PR has been deployed to testing.openlibrary.org for testing label Aug 8, 2023

jimchamp force-pushed the refactor/generic-dropper branch from d4b7492 to 4c2a306 Compare August 9, 2023 17:51

jimchamp added 6 commits September 12, 2023 11:54

Fix updating "saving..." message

fcbfe07

Add more tests

0426419

Fix bugs after rebase

8abd55d

Modify store API

1d21101

Ensure disabled droppers cannot open

acdef29

Check if edition before accessing works list

ce0938f

jimchamp force-pushed the refactor/generic-dropper branch from bc6ed32 to ce0938f Compare September 12, 2023 19:25

jimchamp added 5 commits September 12, 2023 12:29

Increment bundlesize threshold for admin page

cc1dc5a

Remove unhelpful comment

f3bf182

Redirect to login on disabled dropper arrow click

bfa1299

Remove unused data attribute

c02e60f

jimchamp removed the On testing.openlibrary.org This PR has been deployed to testing.openlibrary.org for testing label Sep 16, 2023

jimchamp requested a review from mekarpeles September 16, 2023 00:54

mekarpeles approved these changes Sep 16, 2023

View reviewed changes

jimchamp marked this pull request as draft September 18, 2023 15:26

jimchamp added 5 commits September 19, 2023 08:21

Fix store bugs

0c8bc93

Remove unused class member

e6c31f2

Use list key instead of default cover URL

78c0426

Access data attribute correctly

86d3490

Pass list cover URL to new showcase item

db954ac

jimchamp marked this pull request as ready for review September 19, 2023 16:21

jimchamp added the On testing.openlibrary.org This PR has been deployed to testing.openlibrary.org for testing label Sep 19, 2023

mekarpeles reviewed Sep 19, 2023

View reviewed changes

openlibrary/plugins/openlibrary/js/my-books/index.js Outdated Show resolved Hide resolved

jimchamp and others added 2 commits September 21, 2023 09:00

Update openlibrary/plugins/openlibrary/js/my-books/index.js

a56313d

Co-authored-by: Mek <michael.karpeles@gmail.com>

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

12230de

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

mekarpeles merged commit eab5df4 into internetarchive:master Sep 21, 2023

jimchamp deleted the refactor/generic-dropper branch September 25, 2023 17:47

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

Create My Books Dropper #8019

Create My Books Dropper #8019

jimchamp commented Jun 29, 2023 •

edited

Loading

codecov-commenter commented Jul 4, 2023 •

edited

Loading

mekarpeles Jul 13, 2023

jimchamp Jul 18, 2023

jimchamp Jul 23, 2023

mekarpeles commented Jul 13, 2023

mekarpeles Jul 13, 2023

jimchamp Aug 30, 2023

jimchamp Aug 31, 2023

jimchamp commented Jul 18, 2023 •

edited

Loading

jimchamp Jul 18, 2023 •

edited

Loading

jimchamp Jul 19, 2023

jimchamp commented Jul 27, 2023

cdrini Aug 8, 2023

cdrini Aug 8, 2023

jimchamp commented Sep 16, 2023 •

edited

Loading

mekarpeles left a comment •

edited

Loading

	await createList(this.userKey, data)
	.then(response => response.json())
	.then((data) => {
	this.onListCreationSuccess(data['key'], listTitle)
	})

		@@ -0,0 +1,17 @@
		$def with(primary_button, dropdown_content, additional_classes='')

		$ data_list_key = 'data-list-key=%s' % list.key if actionable else ''
		<li $actionable $data_list_key>

	$ data_list_key = 'data-list-key=%s' % list.key if actionable else ''
	<li $actionable $data_list_key>
	<li $actionable $:cond(actionable, 'data-list-key=%s' % list.key)>

Create My Books Dropper #8019

Create My Books Dropper #8019

Conversation

jimchamp commented Jun 29, 2023 • edited Loading

Technical

The Generic Dropper Component

The My Books Dropper

General

Limitations

Testing

Screenshot

Next steps

Stakeholders

codecov-commenter commented Jul 4, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mekarpeles commented Jul 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimchamp commented Jul 18, 2023 • edited Loading

jimchamp Jul 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimchamp commented Jul 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimchamp commented Sep 16, 2023 • edited Loading

mekarpeles left a comment • edited Loading

Choose a reason for hiding this comment

jimchamp commented Jun 29, 2023 •

edited

Loading

codecov-commenter commented Jul 4, 2023 •

edited

Loading

jimchamp commented Jul 18, 2023 •

edited

Loading

jimchamp Jul 18, 2023 •

edited

Loading

jimchamp commented Sep 16, 2023 •

edited

Loading

mekarpeles left a comment •

edited

Loading