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

Add want to read button to Search Page #5933

Closed

Conversation

Sabreen-Parveen
Copy link
Collaborator

Closes #5501

This PR adds Want to Read Button to search results page.

Technical

  • The reading log dropdown present in the reading logs page has been used again to implement this.
  • When a user is not logged in, the green Want to Read button appears in the search result.
    • This button is disabled, and when the dropdown is clicked, the user is sent to the login page and after login redirected back to the search results page
  • When a user is logged in, the dropdown from the reading log page appears, and user can choose from the four options.
    • The reading log lite button, upon selection removes the item from the list, but for search results, we might want to keep the book present in the result, so I have disabled this behaviour for search results page by applying following condition in ol.js:
               if($searchPage==null) {
                   $self.closest('.searchResultItem').remove();
               }

Testing

Screenshot

User is not logged in:
image

User is logged in:
image

Stakeholders

@mekarpeles @jimchamp

@cclauss cclauss added the Needs: Review This issue/PR needs to be reviewed in order to be closed or merged (see comments). [managed] label Dec 6, 2021
@mekarpeles mekarpeles added Priority: 2 Important, as time permits. [managed] Theme: Reading Log Related to workflows for creating, modifying, displaying a user's reading log. [managed] labels Dec 6, 2021
@mekarpeles mekarpeles added this to the Active Sprint milestone Dec 6, 2021
</select>
</form>
$if ctx.user:
<form method="POST" action="$(work.key)/bookshelves.json?debug=true" class="reading-log-lite $(searchPage)">
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<form method="POST" action="$(work.key)/bookshelves.json?debug=true" class="reading-log-lite $(searchPage)">
<form method="POST" action="$(work.key)/bookshelves.json" class="reading-log-lite $(form_classes)">

The debug query parameter is not needed here.

</a>
</div>
</div>
</div>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
</div>
</div>

Add newline to the end of the file.

<div class="dropit">
<div class="dropper on">
<div class="log-work">
<button class="unactivated" type="submit">$_('Want to Read')</button>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this should be wrapped in a form, similar to the dropper on in the /lists/widget.html template. This will ensure that patrons are directed to the login page when they press the button directly.

Login redirection will be handled by the bookshelf endpoint when this is inside of a form.

@@ -165,10 +166,12 @@
$for work in works:
$ ocaid = work.ia[0] if work.ia else None
$ availability = (work.get('availability') or {}).get('status')
$ users_work_read_status = get_users_read_status(work, username) if work and username else None
$ decorations = macros.ReadingLogButton(work, read_status=users_work_read_status, url=url, searchPage="searchPage")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$ decorations = macros.ReadingLogButton(work, read_status=users_work_read_status, url=url, searchPage="searchPage")
$ decorations = macros.ReadingLogButton(work, read_status=users_work_read_status, url=url, form_classes="search-page")

Update parameter name and use kebab-case for HTML classes and ids.

@@ -96,6 +96,7 @@ export function initReadingListFeature() {
// new shelf to the server and removing the associated item.
// Note that any change to this select will result in the book changing
// shelf.
const $searchPage = $('.searchPage')[0] || null
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const $searchPage = $('.searchPage')[0] || null
const $searchPage = $('.search-page')[0] || null

@@ -104,7 +105,9 @@ export function initReadingListFeature() {
},
datatype: 'json',
success: function() {
$self.closest('.searchResultItem').remove();
if ($searchPage===null) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if ($searchPage===null) {
if (!$searchPage) {

null is falsy, so the condition can be simplified.

Copy link
Collaborator

@jimchamp jimchamp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this is really close.

I'm noticing that it's not clear that a bookshelf has been changed when I select a new value from the drop-down. Can we add the check symbol to the newly selected shelf and remove it from the old shelf? This will probably have to be done via Javascript.

@Sabreen-Parveen
Copy link
Collaborator Author

Sabreen-Parveen commented Dec 11, 2021

I think that this is really close.

I'm noticing that it's not clear that a bookshelf has been changed when I select a new value from the drop-down. Can we add the check symbol to the newly selected shelf and remove it from the old shelf? This will probably have to be done via Javascript.

Added this by reloading only the select element

@jimchamp
Copy link
Collaborator

jimchamp commented Jan 5, 2022

Let's punt this for a bit. This component make sense to use on the reading log pages, as the item is removed from the list when the read status is changed. When this component is used in search results, the "Remove" option doesn't make sense. The checkmarks also need to be added when a new option is selected.

Ideally, the book page "Want to Read" dropper component would be used for both search results and reading log page. Unfortunately, the lists/widget.html template needs to be refactored in order to make this work (the dropper component is in this template, and it looks like it will be difficult to decouple it from the template).

Let's revisit this issue once there is a dedicated template for the "Want to Read" dropper.

@jimchamp jimchamp closed this Jan 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs: Review This issue/PR needs to be reviewed in order to be closed or merged (see comments). [managed] Priority: 2 Important, as time permits. [managed] Theme: Reading Log Related to workflows for creating, modifying, displaying a user's reading log. [managed]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show "Want to Read" button for books on search results page.
4 participants