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

Suggestion list #3420

Draft
wants to merge 47 commits into
base: main
Choose a base branch
from
Draft

Suggestion list #3420

wants to merge 47 commits into from

Conversation

mouse-reeve
Copy link
Member

@mouse-reeve mouse-reeve commented Aug 26, 2024

Description

This PR is meant to complete @joachimesque's work in #2560 -- it lets users suggest "if you like that, try this" suggestions for books using the existing lists functionality.

What type of Pull Request is this?

  • Bug Fix
  • Enhancement
  • Plumbing / Internals / Dependencies
  • Refactor

Does this PR change settings or dependencies, or break something?

  • This PR changes or adds default settings, configuration, or .env values
  • This PR changes or adds dependencies
  • This PR introduces other breaking changes

Details of breaking or configuration changes (if any of above checked)

I'm hoping this won't be breaking per se, but the federation of these lists is going to be a change and that will impact how instances that have the feature interact with those that don't. But I haven't written that part yet so tbd.

Documentation

  • New or amended documentation will be required if this PR is merged
  • I have created a matching pull request in the Documentation repository
  • I intend to create a matching pull request in the Documentation repository after this PR is merged

Tests

  • My changes do not need new tests
  • All tests I have added are passing
  • I have written tests but need help to make them pass
  • I have not written tests and need help to write them

Screenshots

This is what the suggestions panel looks like:
Screenshot 2024-08-27 at 7 52 01 AM

@mouse-reeve mouse-reeve mentioned this pull request Aug 26, 2024
@joachimesque
Copy link
Contributor

🎉 I’ve just seen the PR notification so I haven’t read much, I’ll have to reacclimate myself to the codebase, but I’m very happy that my contribution is useful! I’ll check it out in details when I have more time :)

@joachimesque
Copy link
Contributor

From my first read-through your changes look great, but I haven’t had the time/infra to test the branch :)

@joachimesque
Copy link
Contributor

Just saw the code for endorsements, that’s awesome! I can’t wait to see the finished feature 😄

@mouse-reeve
Copy link
Member Author

mouse-reeve commented Aug 27, 2024

It turned out to be so simple! The one thing I'm dithering on that you might have thoughts about is whether recommendations should be associated with an Edition or with a Work.

The big downside to using Edition is that books with multiple, functionally interchangeable editions (like books with a paperback, hardcover, alternative cover, et cetera) will have separate recommendations. I think that's a pretty big negative, and would make the experience for users who happen to look at the paperback edition one day and the hardcover edition the next pretty weird.

But if it's associated with Work, all the recommendation lists for books that have editions in various languages will get inundated with English-language recommendations, and it will make the experience worse for non-English-language readers.

A third option would be to keep the list with Edition and display recommendations for all Editions under the parent Work -- this is how reviews are displayed. That means it would be technically possible to extend or modify the UI to do something like prioritize suggestions with the same language, or give users the option to filter recommendations to that edition. At this point the experience would be the same as if it was associated with a Work, so it doesn't directly solve any of those problems, but it does give a little more flexibility in the future. It would however make the code a lot more complicated.

@joachimesque Do you have any opinions or alternative ideas?

@mouse-reeve
Copy link
Member Author

The more I think about my third suggestion, the harder it sounds to implement, and I'm not sure it's actually feasible

@joachimesque
Copy link
Contributor

joachimesque commented Aug 27, 2024

(reading that after a night out, so I might not be as sprightly as I could)

I don't know if I understood the third option—it sounded complicated even before I read that it could be very complicated.

Using Work could be the best solution, I hadn’t thought about the restrictions around Edition when I wrote the basis of this PR. My point of view is: even if the suggestions are in another language, I would welcome it. BUT, I’m saying that as a French guy who reads a LOT of English books, so I understand my experience is not most people’s experience.

If Work is used, would it be possible to select the Edition that’s in the language of the person/interface (if it exists)? (Or is this proposal the third option?)

@mouse-reeve
Copy link
Member Author

I had a similar thought, and yes it would be possible to filter or prioritize editions based on language. Outside the scope of this PR, but it gives me some confidence that the potential downsides to using Work could be mitigated in the future.

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.

2 participants