bedrock icon indicating copy to clipboard operation
bedrock copied to clipboard

fix no-JS lang switcher (#5931)

Open Ingi-Hong opened this issue 2 years ago • 17 comments

One-line summary

Fixes the no javascript language switcher that sits in the footer.

Significant changes and points to review

  • added check in l10n_util render function for any a query parameter called lang.

Issue / Bugzilla link

#5931

Testing

disable JS on your browser, try using the language switcher.

Not sure what needs to be looked at to ensure this fully works

Tested on Firefox and Chrome.

  • [x] Tried switching languages on root path /
  • [x] Tried switching languages on a non root path
  • [x] Tried switching from english to cn and the redirect worked.
  • [x] Tried going to non locale page (careers) and manually putting in parameters into the url -- something like localhost:8000/en-US/careers/?lang=da#. page did not break and parameters were removed on refresh.

Ingi-Hong avatar Jul 01 '23 14:07 Ingi-Hong

Closing stale PR, please feel free to reopen should you have time to return to this. Thanks!

alexgibson avatar Aug 16 '23 09:08 alexgibson

I think at this point I was just waiting for a review

edit: ready for review!

Ingi-Hong avatar Oct 19 '23 15:10 Ingi-Hong

Hi Alex, doesn't look like I have the permissions to reopen this PR. I think it's ready for review, if you could reopen

Ingi-Hong avatar Nov 06 '23 15:11 Ingi-Hong

@Ingi-Hong re-opened. If you think this PR is ready for review, can you please update it from draft status? Thanks

alexgibson avatar Nov 06 '23 16:11 alexgibson

@stevejalim no rush on this one, but can you please take a look here? I'm not sure if there are any downside or potential problems with this solution. The other alternative would be to only show the form if JS is enabled (which I think would be fine also fwiw).

alexgibson avatar Nov 06 '23 16:11 alexgibson

@Ingi-Hong whilst you're waiting on review here, I noticed there are some linting errors that need fixing: https://github.com/mozilla/bedrock/actions/runs/6773261630/job/18407712116?pr=13336

alexgibson avatar Nov 09 '23 13:11 alexgibson

@Ingi-Hong - hi, are you still working on this?

alexgibson avatar Jan 15 '24 17:01 alexgibson

Hi Alex, sorry I turned off notifications for this account to test something and forgot to turn it back on. I deleted the line that clears the query per your comment.

Ingi-Hong avatar Jan 20 '24 20:01 Ingi-Hong

Hi @Ingi-Hong - looks like there are some small formatting issues to fix up:

 lib/l10n_utils/__init__.py:119:71: W291 [*] Trailing whitespace
lib/l10n_utils/__init__.py:120:24: Q000 [*] Single quotes found but double quotes preferred
lib/l10n_utils/__init__.py:121:34: Q000 [*] Single quotes found but double quotes preferred
Found 3 errors.
[*] 3 fixable with the `--fix` option.

If you hve pre-commit installed you can install all the hooks we use with pre-commit install-hooks then run them all with pre-commit run -a -- it should catch the things noted above. (Or you can install ruff directly and do ruff . to format just the Python files.) Both approaches will fix the formatting and you can stage, commit and push up the changes. Thanks!

stevejalim avatar Jan 22 '24 11:01 stevejalim

@stevejalim I'll be honest I don't feel like I have a good understanding of the possible repercussions or side effects of this change, give the area of code where this change lives.

Perhaps another back-ender might be able to spot something I'd likely miss?

alexgibson avatar Jan 22 '24 14:01 alexgibson

No worries @alexgibson!

@robhudson I recall you're familiar with our locale-related logic. Could you give this an r, too, with a particular eye open for gotchas of the change, please?

stevejalim avatar Jan 22 '24 14:01 stevejalim

This seems okay at first glance. Basically anywhere we use our own lib.l10n_utils.render we would be able to override the chosen language for that page with a ?lang={lang} query string parameter. Further down in the code, if that language doesn't have a matching translation, it'll redirect.

Since this is a tricky area of our codebase I'd prefer to have tests with this merge to ensure no other repercussions and to also show that this change is intentional.

robhudson avatar Jan 22 '24 16:01 robhudson

Can you suggest any specific files where extra tests could be added @robhudson ?

stevejalim avatar Jan 24 '24 13:01 stevejalim

Can you suggest any specific files where extra tests could be added @robhudson ?

The TestRender class in lib/l10n_utils/tests/test_base.py would be excellent.

robhudson avatar Jan 25 '24 00:01 robhudson

Let me know if you want me to squash the commits

Ingi-Hong avatar Jan 25 '24 03:01 Ingi-Hong

@Ingi-Hong Are you open to adding some tests, too, please? Rob has suggested the file where they would fit:

Can you suggest any specific files where extra tests could be added @robhudson ?

The TestRender class in lib/l10n_utils/tests/test_base.py would be excellent.

If you need any guidance, let me know

stevejalim avatar Jan 29 '24 08:01 stevejalim

Sure, I'd be happy try. I'll take a look when I get the chance.

Ingi-Hong avatar Jan 31 '24 04:01 Ingi-Hong

Closing as per https://github.com/mozilla/bedrock/issues/5931#issuecomment-2003497640

alexgibson avatar Mar 18 '24 14:03 alexgibson