Skip to content
This repository has been archived by the owner on Feb 23, 2024. It is now read-only.

Implement __experimentalCreateInterpolateElement for translations. #1736

Merged
merged 5 commits into from Feb 17, 2020

Conversation

mikejolley
Copy link
Member

@mikejolley mikejolley commented Feb 14, 2020

Following on from #1043, this PR implements the new __experimentalCreateInterpolateElement from the wordpress/element package. It allows us to dynamically insert HTML (in this case anchors) into a translatable string.

For translators, instead of:

Product rating is disabled in your %sstore settings%s.

They will be able to translate a more obvious:

Product rating is disabled in your <a>store settings</a>.

And the link will be inserted. It also means we can avoid using RawHTML and escapeHTML to show string based HTML content.

Since __experimentalCreateInterpolateElement is not available in WordPress yet, only Gutenberg master, this pulls in the package via package.json.

@mikejolley mikejolley requested a review from a team as a code owner February 14, 2020 15:01
@mikejolley mikejolley self-assigned this Feb 14, 2020
@mikejolley mikejolley requested review from nerrad and removed request for a team February 14, 2020 15:01
Copy link
Contributor

@nerrad nerrad left a comment

Choose a reason for hiding this comment

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

Looks good to me Mike. I do have a comment about some attributes added to the links but I'll leave it up to you what direction you go and will pre-approve this. Something to keep in mind is that it looks like we're going to remove the experimental prefix on this function at some point in the near future.

However, it doesn't really impact this work because it's pinned to a version (we can just update when needed).

Also, I confirmed by comparing a build on master with a build on this branch that the bundle-sizes don't really change at all, so that's good!

Let's :shipit:

'admin.php?page=wc-settings&tab=products'
) }
target="_blank"
rel="noopener noreferrer"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we use both noopener and noreferrer here (and in other places in this pull), could we just use noreferrer?

Copy link
Member Author

Choose a reason for hiding this comment

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

I misread the linting rule and thought both were required. Referer could be useful (?) so Ill use noopener.

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm actually the rule does enforce both. eslint(react/jsx-no-target-blank) You cannot just use one or the other...

Copy link
Contributor

@nerrad nerrad Feb 17, 2020

Choose a reason for hiding this comment

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

🤔 Interesting, from this it appears this targets both because of older browser support (but in this context we probably want noreferrer anyways). I can't think of any reason offhand why doing both creates a problem, so let's just keep it as is for now.

Copy link
Contributor

@nerrad nerrad Feb 17, 2020

Choose a reason for hiding this comment

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

Looks like there's an issue that got started around this rule enforcing both :)

Copy link
Contributor

Choose a reason for hiding this comment

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

For reference: a similar discussion happened in Storefront repo. In the end we ended up using rel="noreferrer". We don't have this ESLint rule so it was possible to use only one in that case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the links. I think we're okay using both to keep eslint happy until that issue is fixed. There doesn't appear to be consequences in doing so.

assets/js/blocks/reviews/edit-utils.js Outdated Show resolved Hide resolved
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

5 participants