Add migration notice and translation banner to the blank layout; fix translation banner logic #904

obulat · 2022-02-18T07:50:45Z

Fixes

Description

This PR:

adds the migration notice and translation banner to the blank layout (used on the homepage and 404)
changes the logic to detect whether we should show the translation banner. Since we moved the locale detection to path-based, we remove the WordPress theme - Nuxt locale passing messages, and use the app i18n locale to detect whether we should show the banner.
adds the simple e2e tests for the translation banner.

Testing Instructions

Run the app and go to http://localhost:8443/ru/. You should be able to see the translation banner at the top. From there, you can go to the Russian Openverse translation page, or close the banner. If you navigate to other pages, you should not see the banner after closing it once. Also, you shouldn't see the banner at http://localhost:8443.

Checklist

My pull request has a descriptive title (not a vague title like Update index.md).
My pull request targets the default branch of the repository (main) or a parent feature branch.
My commit messages follow best practices.
My code follows the established code style of the repository.
I added or updated tests for the changes I made (if applicable).
I added or updated documentation (if applicable).
I tried running the project locally and verified that there are no visible errors.

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

Add MigrationNotice and TranslationBanner to the blank layout

# Conflicts: # src/components/VMigrationNotice.vue

sarayourfriend

LGTM! Works well. Just a couple non-blocking comments. Glad we can fix this 🎉

Should we also create a follow-up issue to resolve once this and the theme updates are all deployed to remove any vestiges of the cookie based local stuff? For example I think en_US might be redundant with en in that check as we're never given the wpLocale, just the slug from the theme.

sarayourfriend · 2022-02-18T15:48:14Z

src/composables/use-i18n-sync.js

  /**
   * Show banner inviting to contribute translations if fewer than 90%
   * of strings are translated for current locale.
   * Hard-coded to false for default locales: `en` and `en_US`.
   *
-   * @param {import('../locales/scripts/types').i18nLocaleProps} locale - current locale object
+   * @param {string} localeCode - the wpLocale code for the current locale


nit: It won't be the wp_locale, it'd be the locale slug. wp_locale refers to the en_US, pt_BR version. The slug is what we use 🤷

This is actually not a nit, as there are at least three pieces to each locale that are important here, and we should be sure to select the correct one.
In the wporg_locales.sql you can see that each locale has locale and subdomain properties. locale corresponds to the wp_locale property in the valid-locales.json file. This is different from the key (code) that we use for the locale in Vue-i18n (valid-locales.json file), and is the same as the slug in the GlotPress list of locales where we originally take the data from. And that slug, in turn, is different from the subdomain that is used on WordPress.org.
This is really complicated, and all the values are just a little bit different from one another for some locales.

So, what does locale_slug mean in this case, @sarayourfriend ? Is it the same as the subdomain in the wporg_locales.sql, or slug from the (GlotPress list)[https://github.com/GlotPress/GlotPress-WP/blob/develop/locales/locales.php]?
You can see the difference if you check Venezuelan Spanish: it's wporg subdomain is ve, slug in GllotPress is es-ve, wpLocale is es_VE, and the HTML lang code for it should be es-ve (although ve.wordpress.org site uses es)

So if you look at the GP_Locales class in the locales plugin for wp.org it's a little easier to see what I mean. wporg_locales.sql I think just configures the subdomain -> wp_locale mapping.

https://github.com/wordpress/wordpress.org/blob/e38ba4261c11f90443116df4b2e0654410eb8c04/wordpress.org/public_html/wp-content/mu-plugins/pub/locales/locales.php#L929:L938

There you can see that the string we use to match a <locale>.json is the slug property. get_locale() in WordPress returns the wp_locale property. That's why in the PR to update the wporg-openverse theme to forward the locale via the iframe path needed to map the wp_locale to the slug property.

The subdomain is totally separate. The Brazilian locale, for example, is subdomained at br.wordpress.org, but the locale is pt_BR and the slug is pt-br. The subdomains that wp.org uses are really just kind of made up/convenient compared to the proper locale codes they map to. For example, the Brazilian br subdomain actually clashes with the Breton locale code: https://github.com/wordpress/wordpress.org/blob/e38ba4261c11f90443116df4b2e0654410eb8c04/wordpress.org/public_html/wp-content/mu-plugins/pub/locales/locales.php#L410

Weird stuff! 🤷

Thank you for your detailed explanation!
We also use this slug property to create the translate link, as in (https://translate.wordpress.org/locale/{slug}/default/meta/openverse/). When first writing the translation banner logic, I was afraid we would need the subdomain data as well somewhere. But if we only use slug, we can simplify the locale data object that we have :)

sarayourfriend · 2022-02-18T16:10:19Z

test/e2e/translation-banner.spec.js

+test('Can close the translation banner', async ({ page }) => {
+  await page.goto('/ru/search/')
+  await page.click('[aria-label="Закрыть"]')
+
+  const banner = page.locator(
+    'text=The translation for Russian locale is incomplete. Help us get to 100 percent by '
+  )
+  await expect(banner).not.toBeVisible({ timeout: 10 })
+})


Maybe one more test to verify that the banner is automatically hidden on subsequent page loads after dismissal?

zackkrida

LGTM. On the home and 404 pages these elements cause the height of the page to exceed 100vh, but I think that's okay.

I'll make separate issues, but both banners enabled is not particularly attractive (cc @panchovm):

The translation banner styles aren't ideal on mobile, I think it can be made smaller with some adjustment.

Both banners enabled looks pretty ugly, honestly:

src/composables/use-i18n-sync.js

fcoveram · 2022-02-22T12:02:22Z

I will work on both banners to match the WP.org style. Thanks @obulat for the work and @zackkrida for tagging me

obulat added 3 commits February 18, 2022 10:33

Add MigrationNotice and TranslationBanner to the blank layout

eae36a6

Add MigrationNotice and TranslationBanner to the blank layout

Rewrite translation banner logic to use path-based i18n

c32604d

Add e2e tests for translation banner

263d2ae

obulat added 🟨 priority: medium Not blocking but should be addressed soon ✨ goal: improvement Improvement to an existing user-facing feature 🕹 aspect: interface Concerns end-users' experience with the software labels Feb 18, 2022

obulat requested a review from a team as a code owner February 18, 2022 07:50

obulat requested review from zackkrida and stacimc February 18, 2022 07:50

Merge branch 'main' into fix/translation_banner

4a16b8b

# Conflicts: # src/components/VMigrationNotice.vue

obulat mentioned this pull request Feb 18, 2022

Update locale forwarding via path WordPress/wordpress.org#61

Closed

sarayourfriend approved these changes Feb 18, 2022

View reviewed changes

zackkrida approved these changes Feb 18, 2022

View reviewed changes

fcoveram self-assigned this Feb 18, 2022

obulat commented Feb 21, 2022

View reviewed changes

src/composables/use-i18n-sync.js Outdated Show resolved Hide resolved

obulat added 3 commits February 21, 2022 05:32

Update src/composables/use-i18n-sync.js

03af0fa

Fix docstrings

2901128

Improve e2e test

43b113e

obulat merged commit e57d023 into main Feb 21, 2022

obulat deleted the fix/translation_banner branch February 21, 2022 03:25

fcoveram mentioned this pull request Mar 15, 2022

Style update of translation and news banners #1123

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add migration notice and translation banner to the blank layout; fix translation banner logic #904

Add migration notice and translation banner to the blank layout; fix translation banner logic #904

obulat commented Feb 18, 2022 •

edited

Loading

sarayourfriend left a comment

sarayourfriend Feb 18, 2022

obulat Feb 19, 2022

sarayourfriend Feb 19, 2022 •

edited

Loading

obulat Feb 21, 2022

sarayourfriend Feb 18, 2022

zackkrida left a comment

fcoveram commented Feb 22, 2022

Add migration notice and translation banner to the blank layout; fix translation banner logic #904

Add migration notice and translation banner to the blank layout; fix translation banner logic #904

Conversation

obulat commented Feb 18, 2022 • edited Loading

Fixes

Description

Testing Instructions

Checklist

Developer Certificate of Origin

sarayourfriend left a comment

Choose a reason for hiding this comment

sarayourfriend Feb 18, 2022

Choose a reason for hiding this comment

obulat Feb 19, 2022

Choose a reason for hiding this comment

sarayourfriend Feb 19, 2022 • edited Loading

Choose a reason for hiding this comment

obulat Feb 21, 2022

Choose a reason for hiding this comment

sarayourfriend Feb 18, 2022

Choose a reason for hiding this comment

zackkrida left a comment

Choose a reason for hiding this comment

fcoveram commented Feb 22, 2022

obulat commented Feb 18, 2022 •

edited

Loading

sarayourfriend Feb 19, 2022 •

edited

Loading