Add reading log buttons to search results #6159

jimchamp · 2022-02-17T23:23:45Z

Closes #6122, closes #5501

Refactors the list widget, moving the reading log dropper into a separate file and removing inline JS. Adds reading log button to search results.

Technical

In no particular order:

The need for uniquely identifying list widgets is gone. Instead, event listeners are added to specific elements by class. Listeners are added to the droppers and their child elements, and to various owner list items found on the page (the ones that can be removed by clicking the [X] ).

All Javascript and unused code and variables have been removed from list/widget.html. Any jsdef functions have been redefined with def.

The reading log dropper is now in it's own dedicated file. The "Create a new list" model has also been included in the new file.

Some changes have been made to the dropper:

Two hidden inputs have been added to the dropper: the patron's key and a default seed key.
Checkmark span in primary button is always present. Its visibility is toggled when necessary.
Buttons in dropdown no longer dynamically generated. All three are now present, but the button that corresponds to the primary button is hidden. Visibility is toggled on successful form submit.

Testing

Test all list widget functionality on the following page types:

Book page
Search results page
Author page
Subject page

Screenshot

Stakeholders

@mekarpeles @cdrini

jimchamp · 2022-02-23T20:32:51Z

Recommend replacing our ReadingLogButton.html buttons (used in the "My Books" page) with the new reading log dropper when this is merged.

mekarpeles · 2022-02-27T22:05:25Z

I recall there being one issue where selecting a reading log shelf to "Already Read" and then clearing the selection shows a grey btn w/ text "Already Read".

I reviewed the code and nothing looks too dangerous. I suggest us moving forward and adding test cases where we learn it breaks (like the case mentioned above). I don't think we should block on hitting an arbitrary quality/coverage number.

cdrini

Nice! Code looks headed in a good direction :)

Testing:

❌ Broken on IE11 with Object doesn't support property or method 'closest' -- use jquery ?
- Not a blocker; but could cause issues for other browsers to :/ Create an issue to keep track of it.
✅ Adding book to CR works from search
✅ Clicking button works + no refresh
✅ Deleting works
✅ Adding work vs edition works
Small regression: previously the user's lists would filter based on whether the "work" checkbox was ticked. It doesn't do that anymore. Not a blocker.

Blockers:

Add a unit test to grease the flow, as discussed on call
Removing the bit of HTML inside the JS would be great to keep our i18n healthy and avoid unescaped html

openlibrary/macros/ReadingLogDropper.html

cdrini · 2022-03-01T20:13:25Z

openlibrary/macros/ReadingLogDropper.html

+              <input type="hidden" id="edition_id" name="edition_id" value="$(edition_key)"/>
+            <input type="hidden" id="work_id" name="work_id" value="$(work_key)"/>


These should not have IDs anymore, since they can appear in multiple places in the page. Update any downstream code that refers to these.

Test: Adding work, or edition to list both still work.

Droppers are referenced only by class name in the JS. These IDs no longer serve a function.

openlibrary/macros/ReadingLogDropper.html

openlibrary/plugins/openlibrary/js/lists/index.js

openlibrary/macros/ReadingLogDropper.html

openlibrary/plugins/openlibrary/js/lists/index.js

cdrini · 2022-03-01T20:38:25Z

openlibrary/templates/lists/widget.html

+                    <input type="hidden" name="seed-title" value="$(seed_info['title'])"/>
+                    <input type="hidden" name="seed-key" value="$(seed_info['seed']['key'])"/>
+                    <input type="hidden" name="seed-type" value="$(seed_info['type'])"/>


Where is seed_info coming from? Is it in the global scope? Pass it in as an argument; we want to keep these fns encapsulated and relying less on global scope, to aid in the fact that they can be rendered multiple times now. That'll also let you use this as a jsdef.

seed_info is coming from line 64.

cdrini · 2022-03-01T20:38:36Z

openlibrary/plugins/openlibrary/js/lists/index.js

+    const splitKey = listKey.split('/')
+    const userKey = `/${splitKey[1]}/${splitKey[2]}`
+    const itemMarkUp = `<span class="image">
+          <a href="${listKey}"><img src="${coverUrl}" alt="Cover of: ${listTitle}" title="Cover of: ${listTitle}"/></a>


listTitle needs to be html escaped, potential XSS issue. Use the jsdef from the HTML ; it's available as a global here (or on the window object; e.g. window.show_list(...)). It might seem messy but honestly it's a good compromise and keeps things DRY. It also let's the form be i18n'd, since that's not supported yet for HTML in JS.

Can I just use websafe, which has already been imported?

That will fix the html escaping issue, but won't help with the i18n or the code duplication. But I think it's good enough for now 👍

Also, not sure if that was happening before. Will this change cause issues for folks?

Agree that it should be done, just trying not to mess up anybody's lists.

Oh actually IIRC correctly jsdef doesn't i18n correctly anyways :( So that won't help. The only benefit here would be DRY'ness.

show_list is not global. I18n strings made available client-side via hidden input.

Oh, it's only global if it's a $jsdef and the corresponding HTML is before the JS (which it should be).

openlibrary/templates/lists/widget.html

- Remove dead code and unused variables - Change `jsdef` functions to "regular" functions

Co-authored-by: Drini Cami <cdrini@gmail.com>

cdrini

Lgtm! Now also works in IE11 -- so hopefully all browsers! Nice!

cdrini · 2022-03-03T23:08:00Z

openlibrary/templates/lists/widget.html

-        </div>
-    </div>
+$def i18n_input():
+  $ show_list_i18n_strings = {


Nice! This is a good compromise.

mekarpeles · 2022-03-03T23:11:23Z

🚀

This was referenced Feb 18, 2022

EPIC: Remove inline JS across all python templates #4474

Closed

Move JavaScript for list feature into webpack #2468

Closed

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

jimchamp marked this pull request as ready for review February 19, 2022 03:12

mekarpeles self-assigned this Feb 22, 2022

mekarpeles requested a review from cdrini February 22, 2022 20:47

mekarpeles added the Priority: 1 Do this week, receiving emails, time sensitive, . [managed] label Feb 22, 2022

jimchamp force-pushed the wanna-read branch 2 times, most recently from bfd55b0 to f8ade40 Compare March 1, 2022 17:57

jimchamp mentioned this pull request Mar 1, 2022

Update GitHub Actions #6215

Merged

cdrini requested changes Mar 1, 2022

View reviewed changes

jimchamp and others added 14 commits March 1, 2022 17:15

Add want to read buttons to search results

c7eb302

Remove user lists from dropper

fcf36e4

Remove inline Javascript

a452cbc

Fix CSS linting error

e2fb66e

Ensure button has margin

87836c3

Prevent redirect on reading log update

6168b2d

Prevent 303's when updating reading log

cbe8b7c

Clean up list widget

35a43de

- Remove dead code and unused variables - Change `jsdef` functions to "regular" functions

Add documentation

985f0c4

Use websafe for new list descriptions

f3f99a9

Add bundle size + reduce test coverage

88ae64f

Update test coverage

db9ebcc

Apply some suggestions from code review

a54316c

Co-authored-by: Drini Cami <cdrini@gmail.com>

Add closest() polyfill

b8e08af

jimchamp force-pushed the wanna-read branch from 1b289ce to b8e08af Compare March 1, 2022 22:16

jimchamp added 2 commits March 3, 2022 15:22

Use i18n strings client-side

f65562d

Add test case

a8e31b5

jimchamp requested a review from cdrini March 3, 2022 20:28

cdrini approved these changes Mar 3, 2022

View reviewed changes

mekarpeles merged commit 94488c7 into internetarchive:master Mar 3, 2022

jimchamp mentioned this pull request Apr 8, 2022

Add Reading Log button directly on search results #2716

Closed

jimchamp deleted the wanna-read branch July 15, 2022 23:14

jimchamp mentioned this pull request Aug 23, 2022

Lists in list dropper not populating correctly for works #6897

Closed

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

Add reading log buttons to search results #6159

Add reading log buttons to search results #6159

jimchamp commented Feb 17, 2022 •

edited

Loading

jimchamp commented Feb 23, 2022

mekarpeles commented Feb 27, 2022

cdrini left a comment

cdrini Mar 1, 2022

jimchamp Mar 1, 2022 •

edited

Loading

cdrini Mar 1, 2022

jimchamp Mar 1, 2022 •

edited

Loading

cdrini Mar 1, 2022

jimchamp Mar 1, 2022

cdrini Mar 1, 2022 •

edited

Loading

jimchamp Mar 1, 2022

cdrini Mar 1, 2022

jimchamp Mar 3, 2022

cdrini Mar 3, 2022

cdrini left a comment •

edited

Loading

cdrini Mar 3, 2022

mekarpeles commented Mar 3, 2022

		<input type="hidden" id="edition_id" name="edition_id" value="$(edition_key)"/>
		<input type="hidden" id="work_id" name="work_id" value="$(work_key)"/>

Add reading log buttons to search results #6159

Add reading log buttons to search results #6159

Conversation

jimchamp commented Feb 17, 2022 • edited Loading

Technical

Testing

Screenshot

Stakeholders

jimchamp commented Feb 23, 2022

mekarpeles commented Feb 27, 2022

cdrini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimchamp Mar 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimchamp Mar 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdrini Mar 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdrini left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mekarpeles commented Mar 3, 2022

jimchamp commented Feb 17, 2022 •

edited

Loading

jimchamp Mar 1, 2022 •

edited

Loading

jimchamp Mar 1, 2022 •

edited

Loading

cdrini Mar 1, 2022 •

edited

Loading

cdrini left a comment •

edited

Loading