-
Notifications
You must be signed in to change notification settings - Fork 218
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 i18n placeholder replacement and make tests robust #5343
Conversation
Latest k6 run output1
Footnotes
|
d8eb4fc
to
c206fc2
Compare
Full-stack documentation: https://docs.openverse.org/_preview/5343 Please note that GitHub pages takes a little time to deploy newly pushed code, if the links above don't work or you see old versions, wait 5 minutes and try again. You can check the GitHub pages deployment action list to see the current status of the deployments. Changed files 🔄: |
The placeholders containing single quote (`{'Openverse'})` were not converted to pot Some incorrect placeholders were sent to GlotPress, and translation jsons contain {''} instead of ### ### placeholders Fix vue-i18n link Signed-off-by: Olga Bulat <obulat@gmail.com>
Signed-off-by: Olga Bulat <obulat@gmail.com>
Signed-off-by: Olga Bulat <obulat@gmail.com>
Signed-off-by: Olga Bulat <obulat@gmail.com>
Signed-off-by: Olga Bulat <obulat@gmail.com>
Signed-off-by: Olga Bulat <obulat@gmail.com>
Signed-off-by: Olga Bulat <obulat@gmail.com>
Signed-off-by: Olga Bulat <obulat@gmail.com>
Signed-off-by: Olga Bulat <obulat@gmail.com>
Signed-off-by: Olga Bulat <obulat@gmail.com>
Signed-off-by: Olga Bulat <obulat@gmail.com>
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.
I'm still wrapping my head around the lockfile moving and copying but here are some small nits and questions so far.
@@ -64,7 +64,7 @@ const updateRefs = () => { | |||
} | |||
|
|||
onMounted(updateRefs) | |||
watch(() => [darkMode.colorMode.value, darkMode.osColorMode.value], updateRefs) | |||
watch([darkMode.colorMode, darkMode.osColorMode], () => updateRefs()) |
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.
How is () => updateRefs()
different from updateRefs
? Does it fix a problem or is it a stylistic/conventional thing?
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.
I'm not certain. I added this change trying to fix the cases where Playwright would select an option, but the selected option was still the same on the next line. I think the previous way has one more layer of indirection than what is here, and maybe the change happens in a different tick of the event loop so that Playwright sees the not-yet-changed version?
Update: sorry, all of the above refers to changing from ()=>[computed.value]
to [computed]
With the function, the change should be reverted. I added it to add logs around it
frontend/test/proxy.js
Outdated
console.log(req.method, req.url, tape.res?.status, context.id) | ||
// console.log(req.method, req.url, tape.res?.status, context.id) |
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.
Nit: Why not remove the line?
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.
Updated to only log if verbose
is set to true. This could be very useful for debugging CI failures.
Co-authored-by: Dhruv Bhanushali <hi@dhruvkb.dev>
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.
I have one question about the lock-file movement.
frontend/Dockerfile.playwright
Outdated
# Copy the pnpm-lock.yaml from the root of the monorepo | ||
COPY ../pnpm-lock.yaml ./pnpm-lock.yaml | ||
|
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.
Why does this copy pnpm-lock.yaml
file from the parent directory when the ci_cd.yml
file has copied it into frontend/
? Also how is this Dockerfile able to access files from one level up? As per my understanding, it would not be part of the Docker context.
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.
Updated to copy from frontend directory to docker.
Signed-off-by: Olga Bulat <obulat@gmail.com>
Signed-off-by: Olga Bulat <obulat@gmail.com>
This reverts commit 48e31aa.
Signed-off-by: Olga Bulat <obulat@gmail.com>
Signed-off-by: Olga Bulat <obulat@gmail.com>
Fixes
Follow up fixes after #5261 and #5323
Description
The main reason for this PR was to fix the CI failures. The visual regression tests failed because the locale fallback mechanism broke, so when the locale (Arabic for RTL) string wasn't available, instead of falling back to the English, the i18n keys were used.
The cause was the update of Nuxt i18n and Nuxt in the CI. Playwright tests in CI did not use the locked dependencies from
pnpm-lock.yaml
, and were resolving dependencies differently. So, instead of the Nuxt 3.15.1 and@nuxtjs/i18n
9.1.1., 3.15.2 and 9.1.2 were being installed (check the installed dependencies in this failed run: https://github.com/WordPress/openverse/actions/runs/12894308773/job/35973352713?pr=5341). This combination somehow broke the fallback mechanism.In this PR, I added a step to copy
pnpm-lock.yaml
to the Playwright container to ensure that the locked dependencies are installed.Other fixes
i18n static placeholders
#5261 has replaced
{openverse}
placeholders with{'Openverse'}
, but failed making the corresponding changes to the pot creation script. So, instead of###openverse###
, GlotPress displayed{'Openverse'}
. The converted localejson
files contained illegal characters. This was caught by eslint in PRs.fetchState props
#5323 left out the
canLoadMore
prop fromaudio/collection.vue
, didn't update theCollectionLink
story props, and left a stray console.log statement.App setup on server request
Locally, sometimes you can get a text stats response from the API instead of json, and this breaks the setup because stats are called on every SSR request. To handle that, I updated the plugin order:
init
plugin only sets up the app using available data (cookies)init-stores
plugin is moved later in the initialization chain, so that the error handling and analytics plugins are already available.This PR also makes the Playwright tests more robust by adding the color mode cookies updating the selectors for changing the color mode, and replacing the getter in the watcher with a computed (to make the change faster).
Testing Instructions
The CI should pass.
Checklist
Update index.md
).main
) or a parent feature branch.ov just catalog/generate-docs
for catalogPRs) or the media properties generator (
ov just catalog/generate-docs media-props
for the catalog or
ov just api/generate-docs
for the API) where applicable.Developer Certificate of Origin
Developer Certificate of Origin