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

In NoRent preview, show letter in Spanish and English if needed. #1502

Merged
merged 7 commits into from
May 29, 2020

Conversation

toolness
Copy link
Collaborator

@toolness toolness commented May 28, 2020

This adds special content to the NoRent letter builder page for the case where the user's native language isn't English. All new copy and visuals are based on the Figma mocks.

Below is what the partly-translated version looks like. Strings added in this PR, and others added in very recent PRs (such as the letter and email localization done in #1496) are still in English and/or just show their ids, so it's a bit confusing, but at least it's better than nothing:

image

This can also be manually tested using our language garbler by running node localebuilder.js --garble && yarn lingui:compile.

Note that the English text at one point says "(name of your language) translation" but this text will never actually appear in English, only in the user's language, and it's meant to say e.g. "Spanish translation", in Spanish, if the user's locale is Spanish. I've tried adding a description prop to the <Trans> tag for this to help localizers understand how to localize it, but it is definitely a weird situation, so any alternative strategies are appreciated.

Comment on lines +73 to +75
const Microcopy: React.FC<{ children: React.ReactNode }> = (props) => (
<p className="is-uppercase is-size-7">{props.children}</p>
);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's possible we're really using React instead of CSS here, but it sorta makes the code more readable?

Copy link
Member

Choose a reason for hiding this comment

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

Hmmm yeah this is a tough one. I personally don't like to make react components that are just purely styling related, because when I see the custom component I would assume that it's doing something more complicated than just styling the element, and would then need to take the extra step of tracing back to the definition of the component to verify. But, of course this does make things more DRY and everyone has their own preferences. Totally your call on this!

Comment on lines +136 to +141
<Trans description="heading of formal letter">
<dt>To</dt>
<LandlordAddress {...props} />
<dt>From</dt>
<Address {...props} />
</Trans>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@sraby, I realized that we do actually store the entire localized letter on the server-side for future presentation to the user, when they want to view their archived letters (this was added in #1473 if you remember reviewing that). So I decided to follow your advice in #1496 (comment) and make the whole thing a <Trans> to provide better context, but I also factored out <LandlordAddress> and <Address> to make it less tag-soupy for translators.

Copy link
Member

Choose a reason for hiding this comment

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

Nice! Yeah I think this is a perfect middle ground!

Comment on lines +91 to +94
.jf-letter-translation ul li {
font-weight: inherit;
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ew, I don't like it when I have to "undo" another rule's styling just to get something to look like one would expect it to... At least it's right next to the rule it's overriding though!

Copy link
Member

Choose a reason for hiding this comment

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

Ugh yeah that is annoying to have to do. I've tried using the :not() selectors as much as possible for things like that but that seems very hard to try and maneuver in this scenario!

@toolness toolness changed the title In NoRent preview, letter in Spanish and English if needed. In NoRent preview, show letter in Spanish and English if needed. May 28, 2020
@toolness toolness mentioned this pull request May 28, 2020
33 tasks
@toolness
Copy link
Collaborator Author

Ok @sraby I'm going to merge this now so we can hopefully get the Spanish version out before June 1, but let me know if there's anything in here you think should be changed and we can follow up on it later!

@toolness toolness merged commit f9e955e into master May 29, 2020
@toolness toolness deleted the letter-preview-ux branch May 29, 2020 10:12
Copy link
Member

@sraby sraby left a comment

Choose a reason for hiding this comment

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

This is great Atul! No major changes whatsoever— just a quick comment about your <Microcopy> component but honestly I wouldn't recommend focusing on that given all the other stuff to do.

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.

2 participants