-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Fix work's add-cover
carousel
#8942
Fix work's add-cover
carousel
#8942
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8942 +/- ##
==========================================
- Coverage 15.96% 15.95% -0.01%
==========================================
Files 89 89
Lines 4710 4712 +2
Branches 821 822 +1
==========================================
Hits 752 752
- Misses 3449 3450 +1
- Partials 509 510 +1 ☔ View full report in Codecov by Sentry. |
6cd2f51
to
5db1db2
Compare
5db1db2
to
136727a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm! Tested:
- ✅ https://testing.openlibrary.org/works/OL262421W/The_Adventures_of_Sherlock_Holmes_12_stories/add-cover works
- ✅ Homepage carousels i18n correctly: https://testing.openlibrary.org/?lang=fr
I'm not sure how we might want to best handle this in the long run; it seems a touch funky that the i18n is optional, in terms of what the interface for this component is? But I think we can resolve that if we ever come across this use case again; I reckon it's pretty niche!
It may be nice if |
Yeah that was my thinking exactly; but I agree it seems like a lot of work just to handle this random one-off carousel :P If we find that we hit this issue again with another carousel, seems worth considering though! |
Closes #8906
Restores "next" and "previous" page buttons to the work
add-cover
carousel.Technical
An input with name
carousel-i18n-strings
does not exist on the work's add-cover page. This caused an error during instantiation when i18n strings were parsed, preventing theCarousel
from being created.Now, we check for the
carousel-i18n-strings
input before attempting to parse its value.Testing
Screenshot
Stakeholders
@seabelis