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

Make QueryCarousels Edition Aware #10472

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

benbdeitch
Copy link
Collaborator

@benbdeitch benbdeitch commented Feb 18, 2025

Closes #7252

As of this moment, carousels link to and display works, rather than specific editions. This prevents users from specifically locating given editions of works on carousels, which is not ideal. This PR alters the functionality of carousels to link to and show covers of editions rather than works.

Technical

This issue originally called for restricting the editions that show to ones matching the user's language. In the name of getting this feature out sooner, that has been left out on Drini's advice, to be achieved in a later PR.

Testing

Create a new page on your local version, and create a Carousel macro. {{QueryCarousel("*")}} should be a functional command.

Screenshot

image
And when clicked:
image

Stakeholders

@jimchamp

@github-actions github-actions bot added the Priority: 2 Important, as time permits. [managed] label Feb 18, 2025
@benbdeitch benbdeitch marked this pull request as draft February 18, 2025 21:51
@codecov-commenter
Copy link

codecov-commenter commented Feb 18, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 17.27%. Comparing base (606faaf) to head (4cc7da1).
Report is 436 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #10472      +/-   ##
==========================================
- Coverage   17.31%   17.27%   -0.04%     
==========================================
  Files          87       87              
  Lines        4835     4845      +10     
  Branches      856      860       +4     
==========================================
  Hits          837      837              
- Misses       3471     3477       +6     
- Partials      527      531       +4     

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

@github-actions github-actions bot added the Needs: Response Issues which require feedback from lead label Feb 19, 2025
@benbdeitch benbdeitch force-pushed the edition-aware-carousels branch from 702a924 to be9f271 Compare February 20, 2025 18:27
@benbdeitch benbdeitch force-pushed the edition-aware-carousels branch from a191464 to b38a1a4 Compare February 20, 2025 21:53
@benbdeitch benbdeitch requested a review from cdrini February 20, 2025 21:59
@benbdeitch
Copy link
Collaborator Author

So, this mostly works so far. I'm still ironing out a few kinks. I've stopped it from breaking the current macro, but haven't yet figured out why the addition of 'lang' causes problems. Also, it doesn't yet show editions, which is a problem.

@jimchamp jimchamp assigned jimchamp and unassigned cdrini Feb 21, 2025
@benbdeitch benbdeitch marked this pull request as ready for review February 22, 2025 18:04
@benbdeitch
Copy link
Collaborator Author

Alright, everything should be good to go here. The commit history is slightly messy, but nothing that should cause a problem. It's been tested on my end, and works fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs: Response Issues which require feedback from lead Priority: 2 Important, as time permits. [managed]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make QueryCarousels edition-aware
4 participants