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

Create i18n extension and add french pack #2840

Merged
merged 12 commits into from
Nov 3, 2020

Conversation

sirugh
Copy link
Contributor

@sirugh sirugh commented Oct 27, 2020

Description

Uses the previous example to construct an extension that provides some maintainable translation packs.

Related Issue

Closes PWA-1071.

Acceptance

Verification Stakeholders

Specification

Verification Steps

  1. Delete venia-ui/i18n/fr_FR.json if you have it.
  2. yarn venia add -D @magento/venia-sample-language-packs@0.0.1
  3. yarn watch:venia using a backend with a french locale enabled.
  4. Use the store switcher to change to French store view. Translations should work.

Screenshots / Screen Captures (if appropriate)

Checklist

  • I have added tests to cover my changes, if necessary.
  • I have added translations for new strings, if necessary.
  • I have updated the documentation accordingly, if necessary.

@@ -52,6 +52,7 @@
"@babel/runtime": "~7.4.2",
"@magento/babel-preset-peregrine": "~1.1.0",
"@magento/eslint-config": "~1.5.0",
"@magento/i18n-defaults": "~0.0.1",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will have to publish the extension to npm before merging this PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would we want to provide every install with a French language pack from an example extension? I thought this language pack was to enable testing when required?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do need a less manual way to provide it for the production deployments of master and develop as we have added a french store view to the backends to demonstrate the functionality. I believe @dpatil-magento has uploaded a copy of the fr_FR.json file to the CI system somewhere but I expect he has to manually update it.

On top of that, I had assumed we would just not include this dependency when scaffolding, but I realize that not everyone uses scaffolding (some just clone/fork) so that's probably not a valid assumption.

Sounds like we need to come up with a way optionally include this dependency, but as you are supposed to include extensions as a dependency of your project, I'm not really sure how we can do that here. Maybe in the extension we can add an environment variable to only include it when necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Silly me, moved this to an opt-in experience. @dpatil-magento can set up the CI to install it. Developers can add it as a dependency (or link it) locally. Just need to make sure we on the core team don't accidentally commit it to venia-concept's package.json!

@PWAStudioBot
Copy link
Contributor

PWAStudioBot commented Oct 27, 2020

Fails
🚫 A version label is required. A maintainer must add one.
Messages
📖

Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.

📖 DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).
📖

Associated JIRA tickets: PWA-1071.

Generated by 🚫 dangerJS against 6a851c1

@@ -52,6 +52,7 @@
"@babel/runtime": "~7.4.2",
"@magento/babel-preset-peregrine": "~1.1.0",
"@magento/eslint-config": "~1.5.0",
"@magento/i18n-defaults": "~0.0.1",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would we want to provide every install with a French language pack from an example extension? I thought this language pack was to enable testing when required?

@larsroettig
Copy link
Member

From my point of view should be an optional dependency and only English should be shipped by default

@sirugh
Copy link
Contributor Author

sirugh commented Oct 28, 2020

@davemacaulay @larsroettig I have removed the dependency from venia-concept and provided installation instructions :)

tjwiebell
tjwiebell previously approved these changes Oct 28, 2020
Copy link
Contributor

@tjwiebell tjwiebell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Straight forward and works as expected, nice work!

FYI - you can get yarn to use your local workspace copy if you specify a version, no need to go through symlinking. This should do it: yarn venia add @magento/i18n-defaults@0.0.1 -D.

Signed-off-by: sirugh <rugh@adobe.com>
Signed-off-by: sirugh <rugh@adobe.com>
Signed-off-by: sirugh <rugh@adobe.com>
Signed-off-by: sirugh <rugh@adobe.com>
Signed-off-by: sirugh <rugh@adobe.com>
@dpatil-magento
Copy link
Contributor

QA Approved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.