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

actions and reactions i18n #15058

Merged

Conversation

yheuhtozr
Copy link
Contributor

@yheuhtozr yheuhtozr commented Oct 13, 2021

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update

Description

Extracts actions or reactions related strings for i18n. Attached fr locale for testing purposes. Existing translations up to #15002 reflected (hopefully).

Related Tickets & Documents

Relates to #14888

QA Instructions, Screenshots, Recordings

UI accessibility concerns?

Added/updated tests?

  • Yes
  • No, and this is why: please replace this line with details on why tests
    have not been included
  • I need help with writing tests

[Forem core team only] How will this change be communicated?

Will this PR introduce a change that impacts Forem members or creators, the
development process, or any of our internal teams? If so, please note how you
will share this change with the people who need to know about it.

  • I've updated the Developer Docs or
    Storybook (for Crayons components)
  • This PR changes the Forem platform and our documentation needs to be
    updated. I have filled out the
    Changes Requested
    issue template so Community Success can help update the Admin Docs
    appropriately.
  • I've updated the README or added inline documentation
  • I've added an entry to
    CHANGELOG.md
  • I will share this change in a Changelog
    or in a forem.dev post
  • I will share this change internally with the appropriate teams
  • I'm not sure how best to communicate this change and need help
  • This change does not need to be communicated, and this is why not: this is ongoing work related to a large, ongoing initiative.

[optional] Are there any post deployment tasks we need to perform?

N/A

[optional] What gif best describes this PR or how it makes you feel?

alt_text

@pr-triage pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Oct 13, 2021
@github-actions
Copy link
Contributor

Thank you for opening this PR! We appreciate you!

For all pull requests coming from third-party forks we will need to
review the PR before we can process it through our CI pipelines.

A Forem Team member will review this contribution and get back to
you as soon as possible!

other: "%{count}%{start}\_réactions%{end}"
title: Reactions
like:
title: Coeur
Copy link
Contributor Author

@yheuhtozr yheuhtozr Oct 13, 2021

Choose a reason for hiding this comment

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

BTW, are French speakers fine with Coeur instead of Cœur? (I just copied from another commit as it is.)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is ok for now, we can do another pass through the files at a later stage when they have settled down a bit more.

@yheuhtozr yheuhtozr force-pushed the patch-actions-reactions-i18n-14888 branch from 4d337d1 to fe2bb6c Compare October 14, 2021 10:53
@juliannatetreault
Copy link
Contributor

Hey @yheuhtozr! 👋 Can you please update your branch with the latest code from the repository’s main branch? i18n files weren’t being loaded properly and we have merged a fix for this. Thanks so much in advance and please do not hesitate to reach out if you need help!

Copy link
Contributor

@juliannatetreault juliannatetreault 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 looking good so far! I have two comments, one that is inline and one that I will mention here: I noticed that there is a fair amount of whitespace beneath the Reading List (please see the attached screenshot for reference) and I don't seem to experience this on main. Are you also experiencing this?
Screen Shot 2021-10-14 at 4 17 14 PM

Copy link
Contributor

@citizen428 citizen428 left a comment

Choose a reason for hiding this comment

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

I'm approving this before the merge conflicts have been resolved but this looks good so far.

@pr-triage pr-triage bot added PR: partially-approved bot applied label for PR's where a single reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Oct 18, 2021
@pr-triage pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: partially-approved bot applied label for PR's where a single reviewer approves changes labels Oct 21, 2021
@citizen428
Copy link
Contributor

citizen428 commented Oct 21, 2021

There's an issue with views.notifications.reacted.and_other_html in both translation files it seems.

image

@yheuhtozr
Copy link
Contributor Author

@citizen428 Thank you, but it is strange. The key is apparently there in both locale files.

---
en:
views:
notifications:
reacted:
and_html: "%{first} and %{last}"
and_other_html:
other: "%{first} and %{count} others"
verb_html:
other: "%{start}%{actors} reacted to %{title} with%{end} %{reactions}"
your:
article: your article
comment: your comment

@citizen428
Copy link
Contributor

@yheuhtozr See the last column, this is about having a count and not providing a one translation, only an other translation. I know it's a bit weird as we don't really need it but i18n-tasks seems to be strict in this regard.

@pr-triage pr-triage bot added PR: partially-approved bot applied label for PR's where a single reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Oct 21, 2021
@yheuhtozr
Copy link
Contributor Author

@citizen428 BTW is it possible to have i18n-tasks show in the error output which keys are missing?

@citizen428
Copy link
Contributor

Do you mean during the spec run?

@yheuhtozr
Copy link
Contributor Author

Yes, that's what I meant.

@citizen428
Copy link
Contributor

No, and I don't think we'll add that. But: you can make a copy of the file, add it to gitignore, add whatever output you want and run that locally.

@pr-triage pr-triage bot removed the PR: partially-approved bot applied label for PR's where a single reviewer approves changes label Oct 21, 2021
@pr-triage pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Oct 21, 2021
@pr-triage pr-triage bot added PR: partially-approved bot applied label for PR's where a single reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Oct 25, 2021
@juliannatetreault juliannatetreault merged commit ffd8c9e into forem:main Oct 25, 2021
@pr-triage pr-triage bot added PR: merged bot applied label for PR's that are merged and removed PR: partially-approved bot applied label for PR's where a single reviewer approves changes labels Oct 25, 2021
@Zhao-Andy Zhao-Andy mentioned this pull request Oct 25, 2021
1 task
@djuber djuber mentioned this pull request Oct 26, 2021
9 tasks
citizen428 pushed a commit that referenced this pull request Oct 29, 2021
* actions and reactions i18n

* remove ja.yml

* Update en.yml

* Update fr.yml

* Update _actions.html.erb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: merged bot applied label for PR's that are merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants