Enhance/4762 - Implement UI for new AdSense setup main components #4978
Conversation
|
Size Change: +487 B (0%) Total Size: 1.35 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
There's an i18n change to make here, but I noticed the IB mentioned some stories needing updating:
Update
assets/js/modules/adsense/components/setup/v2/SetupMain.stories.jsto include stories where above components are rendered.
But I don't see those changes in this PR. Could you add those stories?
| sprintf( | ||
| /* translators: %s: user email address */ | ||
| __( | ||
| 'Already use AdSense? Add %s as a user to an existing AdSense account.', | ||
| 'google-site-kit' | ||
| ), | ||
| userEmail | ||
| ) }{ ' ' } | ||
| <Link | ||
| href={ supportURL } | ||
| inherit | ||
| external | ||
| aria-label={ __( | ||
| 'Learn more about adding a user to an existing AdSense account', | ||
| 'google-site-kit' | ||
| ) } | ||
| > | ||
| { __( 'Learn more', 'google-site-kit' ) } | ||
| </Link> |
There was a problem hiding this comment.
I know there are a lot of places where we use this sort of "space between two translated strings" approach, but technically it's not quite right. It presents problems when trying to localise this whole section of text properly as they are really one paragraph, but being presented to translators as two distinct pieces of text AND being combined without proper thought given to how a particular locale would combine them. Really we should be doing this:
| sprintf( | |
| /* translators: %s: user email address */ | |
| __( | |
| 'Already use AdSense? Add %s as a user to an existing AdSense account.', | |
| 'google-site-kit' | |
| ), | |
| userEmail | |
| ) }{ ' ' } | |
| <Link | |
| href={ supportURL } | |
| inherit | |
| external | |
| aria-label={ __( | |
| 'Learn more about adding a user to an existing AdSense account', | |
| 'google-site-kit' | |
| ) } | |
| > | |
| { __( 'Learn more', 'google-site-kit' ) } | |
| </Link> | |
| createInterpolateElement( | |
| sprintf( | |
| /* translators: %s: user email address */ | |
| __( | |
| 'Already use AdSense? Add %s as a user to an existing AdSense account. <a>Learn more</a>', | |
| 'google-site-kit' | |
| ), | |
| userEmail, | |
| ), | |
| { | |
| a: <Link | |
| href={ supportURL } | |
| inherit | |
| external | |
| aria-label={ __( | |
| 'Learn more about adding a user to an existing AdSense account', | |
| 'google-site-kit' | |
| ) } | |
| /> | |
| } | |
| ) |
I know it's sort of annoying, but it's the right approach here to give a really solid i18n experience.
@tofumatt, though the IB says to update the stories, it has been already implemented as part of another ticket. You can check out the branch and confirm the components are being rendered :) |
Summary
Addresses issue:
Relevant technical choices
PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist