Fix homepage books carousel shows english text after loading by benbdeitch · Pull Request #8586 · internetarchive/openlibrary

benbdeitch · 2023-12-04T00:15:04Z

This is a fix for an error in the rendering of carousels. When it comes time to retrieve more books past those initially loaded, the Javascript would render the strings in English, regardless of the user's selected language. Additionally, this pull request replaces the 'loading' test with a small SVG, to render it more comprehensible to those traversing the library in other languages.

Technical

This implementation uses a hidden input element in [the custom carousel template(https://github.com/internetarchive/openlibrary/blob/master/openlibrary/templates/books/custom_carousel.html) to provide the data for the Javascript to render.

Testing

To review, add a significant amount ( >18of books to any given carousel, while set to a non-english language. Currently, after a given point, the Carousels revert back to rendering elements with English strings. Now, they should remain uniform throughout.

Screenshot

Stakeholders

@jimchamp

…age-books-carousel-shows-English-text-after-loading

for more information, see https://pre-commit.ci

codecov-commenter · 2023-12-04T00:18:02Z

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (308a35d) 16.51% compared to head (3ee404b) 16.68%.
Report is 177 commits behind head on master.

Files	Patch %	Lines
...ibrary/plugins/openlibrary/js/carousel/Carousel.js	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8586      +/-   ##
==========================================
+ Coverage   16.51%   16.68%   +0.16%     
==========================================
  Files          85       88       +3     
  Lines        4456     4681     +225     
  Branches      782      835      +53     
==========================================
+ Hits          736      781      +45     
- Misses       3227     3384     +157     
- Partials      493      516      +23

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

jimchamp

Thanks @benbdeitch! You'll find some review notes below this comment.

Also, please replace the newly added spinner with localized "Loading..." strings. There is a second "Loading..." message in Carousel.js (near line 201). Please make sure that message is replaced, as well.

jimchamp · 2023-12-09T01:00:53Z

openlibrary/templates/books/custom_carousel.html

+  $ "open": _("Read"),
+  $ "borrow_available": _("Borrow"),
+  $ "borrow_unavailable": _("Join Waitlist"),
+  $ "error": _("Not in Library")


Suggested change

$ "error": _("Not in Library")

$ "error": _("Not in Library"),

$ "loading": _('Loading...')

Add "loading" string.

jimchamp · 2023-12-09T01:02:25Z

openlibrary/plugins/openlibrary/js/carousel/Carousel.js

                if (!loadMore.locked && !loadMore.allDone && isOn2ndLastPage) {
                    loadMore.locked = true; // lock for critical section
-                    slick.addSlide('<div class="carousel__item carousel__loading-end">Loading...</div>');
+                    slick.addSlide('<object type="image/svg+xml" data="/static/images/spinner.svg"></object>');


Suggested change

slick.addSlide('<object type="image/svg+xml" data="/static/images/spinner.svg"></object>');

slick.addSlide('<div class="carousel__item carousel__loading-end">i18nValues['loading']</div>');

Use localized "loading" string here.

jimchamp · 2023-12-09T01:03:46Z

openlibrary/plugins/openlibrary/js/carousel/Carousel.js

        });
-
+        //This loads in i18n strings from a hidden input element, generated in the books/custom_carousel.html template.
+        const i18n_values = JSON.parse($('input[name=\'carousel-i18n-strings\']').attr('value'))


Suggested change

const i18n_values = JSON.parse($('input[name=\'carousel-i18n-strings\']').attr('value'))

const i18nValues = JSON.parse($('input[name="carousel-i18n-strings"]').attr('value'))

Use camel case for JS variable names. Replace escaped single quotes with double quotes.

jimchamp · 2023-12-09T01:04:18Z

static/images/spinner.svg

Remove this spinner.

jimchamp · 2023-12-09T01:06:57Z

openlibrary/templates/books/custom_carousel.html

+  <input type="hidden" name="carousel-i18n-strings" value="$json_encode(show_carousel_i18n_strings)">
+
+
+$if render_once('books/custom_carousel'):


Suggested change

$if render_once('books/custom_carousel'):

$if render_once('books/custom_carousel.i18n-strings'):

Use similar naming conventions as the lists/widget.html example (found here).

…ish-text-after-loading' of github.com:benbdeitch/openlibraryfork into internetarchive#5932-Homepage-books-carousel-shows-English-text-after-loading

for more information, see https://pre-commit.ci

jimchamp

There are quite a few syntax errors that need to be corrected. Please be sure to test your code before pushing it.

Building the JS every time a change is made can be annoying and tedious. You can run docker compose exec web npm run watch to automatically build your JS changes when you save the file that you're working on. It's also very fast.

jimchamp · 2023-12-12T01:01:14Z

openlibrary/templates/books/custom_carousel.html

+  $if render_once('books/custom_carousel.i18n-strings')::
+  $:carousel_i18n_input()


Suggested change

$if render_once('books/custom_carousel.i18n-strings')::

$:carousel_i18n_input()

$if render_once('books/custom_carousel.i18n-strings'):

$:carousel_i18n_input()

Second colon causes an error.
Be sure to indent nested statements.

jimchamp · 2023-12-12T01:10:59Z

openlibrary/plugins/openlibrary/js/carousel/Carousel.js

                    loadMore.locked = true; // lock for critical section
-                    slick.addSlide('<div class="carousel__item carousel__loading-end">Loading...</div>');
-
+                    slick.addSlide('<div class="carousel__item carousel__loading-end">i18nValues[loading"]</div>');


Suggested change

slick.addSlide('<div class="carousel__item carousel__loading-end">i18nValues[loading"]</div>');

slick.addSlide(`<div class="carousel__item carousel__loading-end">${i18nValues[loading"]}</div>`);

Use template literals for string interpolation.

jimchamp · 2023-12-12T01:11:50Z

openlibrary/plugins/openlibrary/js/carousel/Carousel.js

                // Remove the current slides
                slick.removeSlide(totalSlides, true, true);
-                slick.addSlide('<div class="carousel__item carousel__loading-end">Loading...</div>');
+                slick.addSlide('<div class="carousel__item carousel__loading-end">i18nValues["loading"]</div>');


Suggested change

slick.addSlide('<div class="carousel__item carousel__loading-end">i18nValues["loading"]</div>');

slick.addSlide(`<div class="carousel__item carousel__loading-end">${i18nValues["loading"]}</div>`);

Use template literals for string interpolation.

jimchamp · 2023-12-12T01:12:02Z

openlibrary/plugins/openlibrary/js/carousel/Carousel.js

+            open: {cls: 'cta-btn--available', cta: i18n_values['open']},
+            borrow_available: {cls: 'cta-btn--available', cta: i18n_values['borrow_available']},
+            borrow_unavailable: {cls: 'cta-btn--unavailable', cta: i18n_values['borrow_unavailable']},
+            error: {cls: 'cta-btn--missing', cta: i18n_values['error']},


Suggested change

open: {cls: 'cta-btn--available', cta: i18n_values['open']},

borrow_available: {cls: 'cta-btn--available', cta: i18n_values['borrow_available']},

borrow_unavailable: {cls: 'cta-btn--unavailable', cta: i18n_values['borrow_unavailable']},

error: {cls: 'cta-btn--missing', cta: i18n_values['error']},

open: {cls: 'cta-btn--available', cta: i18nValues['open']},

borrow_available: {cls: 'cta-btn--available', cta: i18nValues['borrow_available']},

borrow_unavailable: {cls: 'cta-btn--unavailable', cta: i18nValues['borrow_unavailable']},

error: {cls: 'cta-btn--missing', cta: i18nValues['error']},

i18n_values is no longer defined.

…ish-text-after-loading' of github.com:benbdeitch/openlibraryfork into internetarchive#5932-Homepage-books-carousel-shows-English-text-after-loading

for more information, see https://pre-commit.ci

jimchamp

Thanks @benbdeitch

openlibrary/plugins/openlibrary/js/carousel/Carousel.js

benbdeitch and others added 6 commits November 22, 2023 19:12

Merge branch 'internetarchive:master' into master

556d57b

Merge branch 'internetarchive:master' into internetarchive#5932-Homep…

b8c3dd0

…age-books-carousel-shows-English-text-after-loading

Added SVG for loading on Carousels, circumventing the translation issue.

310ea93

Hidden input for i18n added

229b01d

Added hidden input for i18n string transfer.

9d871cf

[pre-commit.ci] auto fixes from pre-commit.com hooks

2516844

for more information, see https://pre-commit.ci

mekarpeles assigned jimchamp Dec 4, 2023

jimchamp changed the title ~~#5932 homepage books carousel shows english text after loading~~ homepage books carousel shows english text after loading Dec 5, 2023

jimchamp changed the title ~~homepage books carousel shows english text after loading~~ Fix homepage books carousel shows english text after loading Dec 5, 2023

jimchamp requested changes Dec 9, 2023

View reviewed changes

jimchamp added the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label Dec 9, 2023

benbdeitch and others added 3 commits December 10, 2023 16:24

Fixed requested changes

af61f74

Merge branch 'internetarchive#5932-Homepage-books-carousel-shows-Engl…

01bef0d

…ish-text-after-loading' of github.com:benbdeitch/openlibraryfork into internetarchive#5932-Homepage-books-carousel-shows-English-text-after-loading

[pre-commit.ci] auto fixes from pre-commit.com hooks

04700a6

for more information, see https://pre-commit.ci

jimchamp requested changes Dec 12, 2023

View reviewed changes

benbdeitch and others added 3 commits December 13, 2023 18:57

Fixed style errors.

76a4ae8

Merge branch 'internetarchive#5932-Homepage-books-carousel-shows-Engl…

ec3d31c

…ish-text-after-loading' of github.com:benbdeitch/openlibraryfork into internetarchive#5932-Homepage-books-carousel-shows-English-text-after-loading

[pre-commit.ci] auto fixes from pre-commit.com hooks

a6ba555

for more information, see https://pre-commit.ci

jimchamp removed the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label Dec 14, 2023

jimchamp approved these changes Dec 14, 2023

View reviewed changes

openlibrary/plugins/openlibrary/js/carousel/Carousel.js Outdated Show resolved Hide resolved

openlibrary/plugins/openlibrary/js/carousel/Carousel.js Outdated Show resolved Hide resolved

Apply suggestions from code review

3ee404b

jimchamp merged commit 9e76ff8 into internetarchive:master Dec 14, 2023

	$ "error": _("Not in Library")
	$ "error": _("Not in Library"),
	$ "loading": _('Loading...')

	slick.addSlide('<object type="image/svg+xml" data="/static/images/spinner.svg"></object>');
	slick.addSlide('<div class="carousel__item carousel__loading-end">i18nValues['loading']</div>');

	const i18n_values = JSON.parse($('input[name=\'carousel-i18n-strings\']').attr('value'))
	const i18nValues = JSON.parse($('input[name="carousel-i18n-strings"]').attr('value'))

		<input type="hidden" name="carousel-i18n-strings" value="$json_encode(show_carousel_i18n_strings)">


		$if render_once('books/custom_carousel'):

	$if render_once('books/custom_carousel'):
	$if render_once('books/custom_carousel.i18n-strings'):

		$if render_once('books/custom_carousel.i18n-strings')::
		$:carousel_i18n_input()

	slick.addSlide('<div class="carousel__item carousel__loading-end">i18nValues[loading"]</div>');
	slick.addSlide(`<div class="carousel__item carousel__loading-end">${i18nValues[loading"]}</div>`);

Uh oh!

Conversation

benbdeitch commented Dec 4, 2023

Technical

Testing

Screenshot

Stakeholders

Uh oh!

codecov-commenter commented Dec 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jimchamp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimchamp left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimchamp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov-commenter commented Dec 4, 2023 •

edited

Loading

jimchamp left a comment •

edited

Loading