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

js: config.js: Fix newline character handling in data-isso-* i18n strings #992

Merged

Conversation

pkvach
Copy link
Contributor

@pkvach pkvach commented Feb 18, 2024

Checklist

  • All new and existing tests are passing
  • I have added an entry to CHANGES.rst because this is a user-facing change or an important bugfix
  • I have written proper commit message(s)

What changes does this Pull Request introduce?

Replace escaped newline characters in data-isso attribute values with actual newline characters to ensure correct parsing and display.

Why is this necessary?

Fixes #912

Example of a non-working configuration from a related issue:

 <script data-isso="//comments.test.com/" src="//comments.test.com/js/count.min.js"
    data-isso-num-comments-text-en="One comment\n{{ n }} comments"></script>

@jelmer jelmer requested a review from ix5 March 10, 2024 16:31
@jelmer
Copy link
Member

jelmer commented Mar 10, 2024

please complete the checklist in the PR template

@pkvach
Copy link
Contributor Author

pkvach commented Mar 10, 2024

Added an entry to CHANGES.rst

@jelmer
Copy link
Member

jelmer commented Mar 20, 2024

Hmm, not sure what is up with the CI here - can you rebase on main?

@pkvach pkvach force-pushed the fix/handling-i18n-data-attribute-values branch from 0eb1866 to 0039be6 Compare March 20, 2024 18:32
@pkvach
Copy link
Contributor Author

pkvach commented Mar 20, 2024

Sure, I've rebased the changes.

@jelmer
Copy link
Member

jelmer commented Mar 21, 2024

Hmm, some of the actions don't seem to run for this PR?

@pkvach
Copy link
Contributor Author

pkvach commented Mar 21, 2024

I'm not sure if I can get them to launch.

@pkvach
Copy link
Contributor Author

pkvach commented Mar 29, 2024

I think some checks didn't run because there are no changes to the *.py files in this PR.

@jelmer
Copy link
Member

jelmer commented Mar 29, 2024

I think some checks didn't run because there are no changes to the *.py files in this PR.

Ah, good call - fixed in #1004. Can you rebase past that PR?

@pkvach
Copy link
Contributor Author

pkvach commented Mar 29, 2024

@jelmer
Thank you for your help.
It looks like in order for tests to run, changes are needed in this workflow:
https://github.com/isso-comments/isso/blob/6dffaa74f1184280f45a8bf64730f3d153c74b05/.github/workflows/python-tests.yml

@pkvach pkvach force-pushed the fix/handling-i18n-data-attribute-values branch from 0039be6 to 38ebc61 Compare March 30, 2024 22:59
@pkvach
Copy link
Contributor Author

pkvach commented Mar 30, 2024

All checks passed after I rebased onto #1004.

@ix5
Copy link
Member

ix5 commented Apr 13, 2024

Hey @pkvach, thank you for this PR! I see you (were) on a roll here with proposing a lot of useful fixes with great descriptions and tests. Sorry you had to wait so long for review.

I only have one suggestion to make in order to clean up that whole code block while you're at it (in a separate commit). Either way, your code as-is is fine as well.

diff --git a/isso/js/app/config.js b/isso/js/app/config.js
index 8684e8d..4ce92fb 100644
--- a/isso/js/app/config.js
+++ b/isso/js/app/config.js
@@ -17,25 +17,24 @@ for (var i = 0; i < js.length; i++) {
         var attr = js[i].attributes[j];
         if (/^data-isso-/.test(attr.name)) {
 
+            // Normalize underscores to dashes so that language-specific
+            // strings can be caught better later on, e.g.
+            // data-isso-postbox-text-text-PT_BR becomes postbox-text-text-pt-br.
+            // Also note that attr.name only gives lowercase strings as per HTML
+            // spec, e.g. data-isso-FOO-Bar becomes foo-bar, but since the test
+            // environment's jest-environment-jsdom seemingly does not follow
+            // that convention, convert to lowercase here anyway.
+            const attrName = attr.name.substring(10)
+                       .replace(/_/g, '-')
+                       .toLowerCase()
+
             // Replace escaped newline characters in the attribute value with actual newline characters
             const attrValue = attr.value.replace(/\\n/g, '\n');
 
             try {
-                // Normalize underscores to dashes so that language-specific
-                // strings can be caught better later on,
-                // e.g. data-isso-postbox-text-text-PT_BR becomes
-                // postbox-text-text-pt-br.
-                // Also note that attr.name only gives lowercase strings as per
-                // HTML spec, e.g. data-isso-FOO-Bar becomes foo-bar, but since
-                // the test environment's jest-environment-jsdom seemingly does
-                // not follow that convention, convert to lowercase here anyway.
-                config[attr.name.substring(10)
-                       .replace(/_/g, '-')
-                       .toLowerCase()] = JSON.parse(attrValue);
+                config[attrName] = JSON.parse(attrValue);
             } catch (ex) {
-                config[attr.name.substring(10)
-                       .replace(/_/g, '-')
-                       .toLowerCase()] = attrValue;
+                config[attrName] = attrValue;
             }
         }
     }

@pkvach pkvach force-pushed the fix/handling-i18n-data-attribute-values branch from 38ebc61 to 66c7c65 Compare April 14, 2024 05:01
@pkvach
Copy link
Contributor Author

pkvach commented Apr 14, 2024

@ix5
Thank you for your feedback. I appreciate it.
I have made the changes you suggested.

…ings

Replace escaped newline characters in data-isso attribute values with actual newline characters to ensure correct parsing and display.

Fixes isso-comments#912
@pkvach pkvach force-pushed the fix/handling-i18n-data-attribute-values branch from 09a0ba8 to 953051b Compare April 23, 2024 21:42
@ix5 ix5 added client (Javascript) client code and CSS bug labels Apr 23, 2024
@ix5 ix5 added this to the 0.14 milestone Apr 23, 2024
@ix5 ix5 merged commit e3ee8fb into isso-comments:master Apr 23, 2024
14 of 17 checks passed
@ix5
Copy link
Member

ix5 commented Apr 23, 2024

Nice, thank you for your contribution!

@pkvach pkvach deleted the fix/handling-i18n-data-attribute-values branch April 24, 2024 04:50
@ix5 ix5 modified the milestones: 0.14, 0.13.1 May 9, 2024
@Kerumen
Copy link

Kerumen commented Oct 17, 2024

@ix5 Do you think you can release this change? Thanks 🙏🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug client (Javascript) client code and CSS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Overriding translation string fails for count.min.js
4 participants