Skip to content
This repository has been archived by the owner on Jun 1, 2023. It is now read-only.

[Feature] Add l10n support to CheckoutUiExtension #2009

Merged
merged 1 commit into from
Feb 14, 2022

Conversation

ginmrt
Copy link
Contributor

@ginmrt ginmrt commented Feb 3, 2022

WHY are these changes introduced?

Fixes internal issue

WHAT is this pull request doing?

As part of the Localized Checkout Ui Extensions project, we must update the shopify-cli codebase to allow developers to submit localization files along with their code updates when the Shopify CLI shopify extension push command is executed.

This change applies to the Checkout Extension type of extensions only, for now.

Screenshots

Valid submission

This will be what is shown in the CLI when we will have merged in the code in the Shopify backend that will support this new localization feature.
Screen Shot 2022-02-03 at 11 16 08 AM

Valid submission with unrecognized key warning

This will be what is shown in the CLI before and until we will have merged in the code in the Shopify backend that will support this new localization feature.
Screen Shot 2022-02-03 at 11 15 42 AM

Note that attempting to publish an extension with this data pushed in before support has been added will fail.
Screen Shot 2022-02-03 at 10 41 04 AM

Invalid submission: Invalid locale code

This error appears if the locale code of a given file name is invalid.
Screen Shot 2022-02-07 at 4 25 42 PM

Invalid submission: Invalid file extension

This error appears if a file under the /locales folder has an extension type other than .json
Screen Shot 2022-02-03 at 11 08 52 AM

Invalid submission: File too large

This error appears if a localization file is larger than the declared limit
Screen Shot 2022-02-03 at 11 11 15 AM

Invalid submission: File empty

This error appears if a localization file is completely empty
Screen Shot 2022-02-07 at 4 25 24 PM

Invalid submission: Sum of all file sizes too large

This error appears if the combined total size of all localization files exceeds the declared limit
Screen Shot 2022-02-03 at 11 13 06 AM

Invalid submission: No default / too many defaults

This error appears if none of the provided locale files are declared as the default locale, or if more than one of them are.
Screen Shot 2022-02-03 at 11 14 01 AM

How to test your changes?

  • Follow this procedure completely to have a backend to execute the CLI push command against as well as a CheckoutUiExtension extension project setup.
  • Create a locales directory at the root of the created extension project of type Checkout Extension
  • Create .json files with a valid locale code as the basename. Ensure one of them is suffixed by .default, e.g. en.default.json and fr.json
  • Execute the shopify extension push command.

Various simple validations exist, which can be tested by playing around with the filenames under locales and their content. Refer to the screenshots provided above.

Update checklist

  • I've added a CHANGELOG entry for this PR (if the change is public-facing)
  • I've considered possible cross-platform impacts (Mac, Linux, Windows).
  • I've left the version number as is (we'll handle incrementing this when releasing).
  • I've included any post-release steps in the section above.

@ginmrt ginmrt force-pushed the mg/checkout-ui-extensions-localization branch 3 times, most recently from 4de0d27 to f1338d9 Compare February 3, 2022 19:14
@ginmrt ginmrt self-assigned this Feb 3, 2022
@ginmrt ginmrt force-pushed the mg/checkout-ui-extensions-localization branch from f1338d9 to a03df68 Compare February 3, 2022 19:36
@ginmrt ginmrt marked this pull request as ready for review February 3, 2022 20:09
@ginmrt ginmrt requested review from a team, jesalerno84 and gonzaloriestra and removed request for a team February 3, 2022 20:09
Copy link
Contributor

@kumar303 kumar303 left a comment

Choose a reason for hiding this comment

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

Nice work, @ginmrt

Copy link
Contributor

@ecbrodie ecbrodie left a comment

Choose a reason for hiding this comment

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

Left some early feedback. Looks fine overall. I still need to tophat.

@ginmrt ginmrt force-pushed the mg/checkout-ui-extensions-localization branch from a03df68 to 88d6e2b Compare February 4, 2022 15:18
@ecbrodie
Copy link
Contributor

ecbrodie commented Feb 7, 2022

@ginmrt do you mind rebasing this PR onto latest main please? This is necessary for being able to support the Spin rewrite. Thank you.

@ginmrt ginmrt force-pushed the mg/checkout-ui-extensions-localization branch 3 times, most recently from bccaa1d to e961448 Compare February 7, 2022 21:22
Copy link
Contributor

@ecbrodie ecbrodie left a comment

Choose a reason for hiding this comment

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

I 🎩'd with a handpicked list of test scenarios and they all checked out for me. Great work Martin.

There is one additional fix that I am requesting before merge though. Once that is addressed, then I can approve.

@ginmrt ginmrt force-pushed the mg/checkout-ui-extensions-localization branch from e961448 to c936b9e Compare February 8, 2022 15:17
@ginmrt ginmrt requested a review from ecbrodie February 8, 2022 15:24
Copy link
Contributor

@ecbrodie ecbrodie left a comment

Choose a reason for hiding this comment

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

I still have open feedback that should be given consideration. But overall, I've seen enough from this PR to approve.

@ginmrt ginmrt force-pushed the mg/checkout-ui-extensions-localization branch from c936b9e to a189992 Compare February 8, 2022 18:26
@ginmrt
Copy link
Contributor Author

ginmrt commented Feb 10, 2022

Ping @jesalerno84 @gonzaloriestra

Copy link
Contributor

@gonzaloriestra gonzaloriestra left a comment

Choose a reason for hiding this comment

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

I've added a few suggestions, but the only important one would be to extract the error messages. Otherwise, the code and tests look great (I haven't tested it, as it was done before) 👌

@ginmrt ginmrt force-pushed the mg/checkout-ui-extensions-localization branch from a189992 to bcdae00 Compare February 11, 2022 20:45
@ginmrt ginmrt requested a review from kumar303 February 11, 2022 20:45
Copy link
Contributor

@gonzaloriestra gonzaloriestra left a comment

Choose a reason for hiding this comment

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

Thanks for the changes! It looks great 👍

@ginmrt ginmrt merged commit e9668ae into main Feb 14, 2022
@ginmrt ginmrt deleted the mg/checkout-ui-extensions-localization branch February 14, 2022 15:51
@shopify-shipit shopify-shipit bot temporarily deployed to rubygems February 23, 2022 09:55 Inactive
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants