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 elements #15

Merged
merged 5 commits into from
Oct 2, 2023
Merged

Conversation

felicia-haggqvist
Copy link
Contributor

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

To test:

  1. Run pnpm build:elements 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/chevron-down/locales/fi/messages.mjs)

  3. Link this branch to an existing elements-project that has @warp-ds/icon installed as a devDependency and run pnpm build

  4. Use the icon with the altered message in the existing elements-project.* Then run the project, example: <w-icon-chevron-down16></w-icon-chevron-down16>

  5. Inspect the icon in the browser and see if your altered message is there

*Note: Make sure to change the language in the html to the same language where you altered the message in the messages.mjs-file (example: if you changed messages.mjs under the /fi folder, then the language for the html needs to be changed to "fi").

@felicia-haggqvist felicia-haggqvist requested a review from a team October 2, 2023 08:50
@felicia-haggqvist felicia-haggqvist self-assigned this Oct 2, 2023
(attr) => attr.name + `=` + `"` + attr.value + `"`
);
const { message, id, comment } = titleMessage || {};
const titleHtml = "<title>${title}</title>";
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason to have this as a variable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No reason for me. I just implemented the same changes from the scripts/output/react.js-file. But I agree, it can be moved directly to the output-variable on line 37. :)

Copy link
Contributor Author

@felicia-haggqvist felicia-haggqvist Oct 2, 2023

Choose a reason for hiding this comment

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

I think I understand why <title>${title}</title> is in the titleHtml variable. When I try to move <title>${title}</title> to line 37 I get a ReferenceError: "title is not defined". It seems that the title variable on line 35 doesn't have time to be defined. I'm guessing that it's expecting that the title-variable, which is defined on line 35, should be outside of the template-literal from the output-variable. I can either let it be as it is now with that <title>${title}</title> stays in the titleHtml-variable, or I move up line 36 to outside of the output-variable and then use the title-variable directly on line 37. Wdyt? :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I can either let it be as it is now

Yeah, let's do that :)

` render() {
const title = i18n.t({ message: \`${message}\`, id: '${id}', comment: '${comment}' });

return html\`<svg ${attrs.join("")}>${titleHtml}${svg.html}</svg>\`; }`,
Copy link
Contributor

Choose a reason for hiding this comment

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

In my head, it would be more readable if it was <title>${title}</title> instead of titleHtml :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree!

@felicia-haggqvist felicia-haggqvist merged commit e0827e7 into next Oct 2, 2023
1 check passed
@felicia-haggqvist felicia-haggqvist deleted the handle-i18n-for-elements branch October 2, 2023 13:41
felicia-haggqvist added a commit that referenced this pull request Oct 3, 2023
felicia-haggqvist added a commit that referenced this pull request Oct 3, 2023
github-actions bot pushed a commit that referenced this pull request Oct 3, 2023
# [1.1.0-next.5](v1.1.0-next.4...v1.1.0-next.5) (2023-10-03)

### Reverts

* Revert "feat:Handle i18n for elements (#15)" (#18) ([6bfee40](6bfee40)), closes [#15](#15) [#18](#18)
felicia-haggqvist added a commit that referenced this pull request Oct 3, 2023
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.

3 participants