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

Make nonprofit views translatable #1553

Conversation

jvBatista
Copy link
Contributor

Description

Update the nonprofit views and make them translatable

Copy link
Member

@wwahammy wwahammy left a comment

Choose a reason for hiding this comment

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

Thanks so much for your contribution.

So, I really, really like what you're doing and started reviewing it but I think this is too much to review at once. Especially when you're new to an open source project, it's good to submit small PRs.

At first, please translate a single one of these files at first. That way we can make sure everything is going smoothly and then you can ramp up from there.

Thanks so much for your contribution, I'm looking forward to working more with you!

@@ -3,7 +3,7 @@
<!-- partial start: nonprofits/_achievements -->
<% unless @nonprofit.achievements&.join.blank? %>
<aside class='u-marginBottom--15 pastelBox--grey stripedBox u-relative'>
<header>Highlights</header>
<header><%= t('nonprofits.achievements.header') %></header>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<header><%= t('nonprofits.achievements.header') %></header>
<header><%= t('nonprofits.page.achievements.highlights') %></header>

@@ -5,18 +5,18 @@
<aside class='u-marginBottom--15'>
<% if current_nonprofit_user? %>
<button class='button--jumbo u-width--full' open-modal='newCampaign' if-branded='background-color, dark'>
Start a Campaign
<%= t('nonprofits.actions.start_campaign') %>
Copy link
Member

Choose a reason for hiding this comment

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

nonprofits.page.actions.start_campaign

</button>
<% else %>
<a class='button u-width--full' if-branded='background-color, dark' target='_blank' href='/peer-to-peer?npo_id=<%=@nonprofit.id %>'>
Start a Campaign for <%= @nonprofit.name %>
<%= t('nonprofits.actions.start_campaign_for') %> <%= @nonprofit.name %>
Copy link
Member

Choose a reason for hiding this comment

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

One thing that's important about i18n is making sure you use the right abstraction. For example here, in English, the text is Start a Campaign For Nonprofit Name. But in other languages, nonprofit might go first or in a different place in the string. The information on proper abstraction should describe that in https://guides.rubyonrails.org/i18n.html#passing-variables-to-translations.

Also, please change this name to nonprofits.page.actions.start_campaign_for and use nonprofit_name as the name of the argument.

</a>
<% end %>
</button>

<% if current_nonprofit_user? %>
<button class='button--jumbo u-width--full' open-modal='newEvent' data-when-confirmed if-branded='background-color, dark'>
Create an Event
<%= t('nonprofits.actions.create_event') %>
Copy link
Member

Choose a reason for hiding this comment

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

nonprofits.page.actions.create_event

@jvBatista
Copy link
Contributor Author

Thanks so much for your contribution.

So, I really, really like what you're doing and started reviewing it but I think this is too much to review at once. Especially when you're new to an open source project, it's good to submit small PRs.

At first, please translate a single one of these files at first. That way we can make sure everything is going smoothly and then you can ramp up from there.

Thanks so much for your contribution, I'm looking forward to working more with you!

Hello there, first of all I want to thank you for the support and feedback, I really appreciate the review. I'll be sure to follow the recommendations above.

Regarding the suggestion about the size of the PR, should I close this one and open new, smaller ones??

@wwahammy
Copy link
Member

Regarding the suggestion about the size of the PR, should I close this one and open new, smaller ones??

In this case, it would make sense to create a new PR with the changes, link back to this PR in the description of the new PR and then close this PR.

@jvBatista
Copy link
Contributor Author

Following the suggestions above, this thread of changes will continue in future smaller PR's, the first of which is #1557

@jvBatista jvBatista closed this May 23, 2023
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