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

feat: Handle i18n for vue #14

Merged
merged 3 commits into from
Sep 28, 2023
Merged

feat: Handle i18n for vue #14

merged 3 commits into from
Sep 28, 2023

Conversation

felicia-haggqvist
Copy link
Contributor

@felicia-haggqvist felicia-haggqvist commented Sep 26, 2023

Refactor script for generating vue icons to also handle i18n + add translated title for Vue icons (found in scripts/output/vue.js)

To test:

  1. Run pnpm build:vue in this branch
  2. Try to change the message in a messages.mjs-file in one of the icons-folders found under /src/raw (example: /src/raw/info/locales/fi/messages.mjs)
Skärmavbild 2023-09-26 kl  15 42 05
  1. Link this branch to an existing Vue-project that has @warp-ds/icon installed as a devDependency and run pnpm build
  2. Use the icon with the altered message in the existing Vue-project and run the project, example: <icon-info16 />
Skärmavbild 2023-09-26 kl  15 44 18
  1. Inspect the icon in the browser and see if your altered message is there
Skärmavbild 2023-09-26 kl  15 43 13

@felicia-haggqvist felicia-haggqvist requested a review from a team September 26, 2023 13:27
@felicia-haggqvist felicia-haggqvist self-assigned this Sep 26, 2023
getSVGs().forEach(({ svg, filename, exportName }) => {
// Create Vue Icon
getSVGs().forEach(({ svg, filename, exportName, name }) => {
// Handle i18n of icon title
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment for me doesn't add any useful information as the next line is actually to create an icon name and not handle i18n. So maybe it is better to remove this one or move it somewhere else, before line 20 for example. Wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That’s true! I can either move it up to before line 20 or refactor the comment. :)

// Create Vue Icon
getSVGs().forEach(({ svg, filename, exportName, name }) => {
// Handle i18n of icon title
const iconNameCamelCase = exportName.replace('Icon', '').replace(/\d+/g, '');
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a suggestion to simplify but feel free to ignore this one if you don't like it.

Suggested change
const iconNameCamelCase = exportName.replace('Icon', '').replace(/\d+/g, '');
const iconNameCamelCase = exportName.replace(/Icon|\d+/g, '');

Copy link
Contributor

@AnnaRybkina AnnaRybkina left a comment

Choose a reason for hiding this comment

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

Great work 💪 Left two minor comments but looks great and I approve

@felicia-haggqvist felicia-haggqvist merged commit 2cf4fdb into next Sep 28, 2023
1 check passed
@felicia-haggqvist felicia-haggqvist deleted the handle-i18n-for-vue branch September 28, 2023 07:51
github-actions bot pushed a commit that referenced this pull request Sep 28, 2023
# [1.1.0-next.2](v1.1.0-next.1...v1.1.0-next.2) (2023-09-28)

### Features

* Handle i18n for vue ([#14](#14)) ([2cf4fdb](2cf4fdb))
github-actions bot pushed a commit that referenced this pull request Oct 19, 2023
# [1.1.0](v1.0.0...v1.1.0) (2023-10-19)

### Bug Fixes

* add 9BA8BA to exchange with currentColor ([#17](#17)) ([bea742a](bea742a))
* add leading slash to dest in crowdin config ([#23](#23)) ([d1170b6](d1170b6))
* add more missing titles ([#35](#35)) ([91f2b78](91f2b78))
* element icons names ([#16](#16)) ([0d550c8](0d550c8))
* Fix bundling ([#20](#20)) ([6ea9874](6ea9874))
* make sure finnish translations aren't gitignored ([#40](#40)) ([5c49b2b](5c49b2b))
* pnpm lockfile ([6529014](6529014))
* set next as default prerelease branch ([#12](#12)) ([bb0be72](bb0be72))
* update crowdin.yaml with leading slash ([bd09d68](bd09d68))
* update Readme text ([156d4ed](156d4ed))
* update script with improved destination in crowdin ([#34](#34)) ([30f4c92](30f4c92))
* use proper language placeholder in translations ([#22](#22)) ([4425b66](4425b66))

### Features

* Generate individual icons ([#39](#39)) ([dd74394](dd74394))
* Handle i18n for elements ([#19](#19)) ([8a27850](8a27850))
* Handle i18n for vue ([#14](#14)) ([2cf4fdb](2cf4fdb))
* i18n for react ([#9](#9)) ([10e41bc](10e41bc))

### Reverts

* Revert "feat:Handle i18n for elements (#15)" (#18) ([6bfee40](6bfee40)), closes [#15](#15) [#18](#18)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants