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

views/comments i18n #15059

Merged
merged 14 commits into from Oct 27, 2021
Merged

views/comments i18n #15059

merged 14 commits into from Oct 27, 2021

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 strings for i18n from app/views/comments and related. 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!

@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

@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.

Approving pending the merge conflict resolution.

@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
@yheuhtozr
Copy link
Contributor Author

@citizen428 Do you perhaps mean pending conflicts resolved by me? If so, it says "conflicts are too complex to resolve in the web editor" and I don't commit to my GitHub repo directly.

@citizen428
Copy link
Contributor

Do you perhaps mean pending conflicts resolved by me?

Yes, I did. 😅

I don't commit to my GitHub repo directly

I'm not sure I understand.

@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 19, 2021
@yheuhtozr
Copy link
Contributor Author

yheuhtozr commented Oct 19, 2021

@citizen428 My GitHub repo is just a mirror of GitLab. I tried to sync with the main branch but GitHub does not recognize it as resolution and still showing conflicts. Even though I can solve it here, it will be reverted at the next time I push anything to my GitLab repo. Hopefully now the conflicts are simple - you only have to accept all my changes, so if you don't mind, please resolve them immediately before you merge this PR.

BTW the reconciliation commit is rather big that you might want to check it again.

@yheuhtozr yheuhtozr force-pushed the patch-comments-i18n-14888 branch 2 times, most recently from 3cda498 to 15b74fa Compare October 20, 2021 09:12
@citizen428
Copy link
Contributor

@yheuhtozr The spec errors here look legit, maybe something went wrong during the merge?

@yheuhtozr
Copy link
Contributor Author

@citizen428 The template went through a rather complex rewrite that I can't pin down the error source for now. It might need a repush, so you can look for other PRs ready to merge if you don't mind.

@citizen428
Copy link
Contributor

Thanks for letting me know @yheuhtozr!

@yheuhtozr
Copy link
Contributor Author

@citizen428 The last error Travis reports is two missing keys, but it doesn't miss anything on my local environment. Could you let me know which ones specifically?

@citizen428
Copy link
Contributor

citizen428 commented Oct 25, 2021

@yheuhtozr Sure, the one key for views.comments.parent.view_full for both en and fr.

@yheuhtozr
Copy link
Contributor Author

yheuhtozr commented Oct 25, 2021

@citizen428 Now it fails with a mysterious error in a newly added(?) test, which doesn't look relevant to my changes?

@citizen428
Copy link
Contributor

@yheuhtozr Looking into it.

Co-authored-by: Michael Kohl <citizen428@forem.com>
@citizen428
Copy link
Contributor

@yheuhtozr Re-ran the specs, they are green now.

@citizen428 citizen428 requested review from aitchiss and removed request for rhymes October 26, 2021 06:00
Copy link
Contributor

@aitchiss aitchiss left a comment

Choose a reason for hiding this comment

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

Thanks for taking on these changes! I noticed just a couple of small issues, but otherwise all looking great 🔥

app/views/articles/_single_story.html.erb Show resolved Hide resolved
app/views/comments/_comment_date.erb Outdated Show resolved Hide resolved
<li class="mod-actions hidden mod-actions-comment-button" data-path="<%= URL.comment(comment) %>/mod" aria-label="Moderate <%= comment.user.name %>'s comment"></li>
<li class="report-abuse-link-wrapper" data-path="/report-abuse?url=<%= URL.comment(comment) %>" aria-label="Report <%= comment.user.name %>'s comment as abusive or violating our code of conduct and/or terms and conditions"></li>
<li class="mod-actions hidden mod-actions-comment-button" data-path="<%= URL.comment(comment) %>/mod" aria-label="<%= t("views.comments.menu.moderate.aria_label", user: comment.user.name) %>"></li>
<li class="report-abuse-link-wrapper" data-path="/report-abuse?url=<%= URL.comment(comment) %>" aria-label="<%= t("views.comments.menu.report.aria_label", user: comment.user.name) %>"></li>
Copy link
Contributor

Choose a reason for hiding this comment

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

(not blocking at all) Nice one adding the aria-label translations here - it's also made me notice that the labels aren't actually being attached to the inner buttons/links, only to the list item, so they're not having the desired effect.

This is still great though, and sets us up well to fix that in a later issue as it's nothing to do with your changes. Just wanted to say yay for bringing it to our attention!

@yheuhtozr yheuhtozr mentioned this pull request Oct 27, 2021
16 tasks
Copy link
Contributor

@aitchiss aitchiss left a comment

Choose a reason for hiding this comment

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

Thanks for making that small fix!

@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 27, 2021
@aitchiss aitchiss merged commit f762acc into forem:main Oct 27, 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 27, 2021
citizen428 added a commit that referenced this pull request Oct 29, 2021
* views/comments etc i18n

* comments PR fixes

* PR sync with main

* remove ja.yml

* Update _comment_header.html.erb

* Update index.html.erb

* Update _comment_header.html.erb

* Update en.yml

* Update fr.yml

* Update config/locales/views/comments/en.yml

Co-authored-by: Michael Kohl <citizen428@forem.com>

* Update _comment_date.erb

* Update en.yml

* Update fr.yml

Co-authored-by: Michael Kohl <citizen428@forem.com>
@yheuhtozr yheuhtozr mentioned this pull request Feb 11, 2023
16 tasks
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

4 participants