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

I18n for Comments #5458

Merged
merged 2 commits into from
Aug 25, 2018
Merged

Conversation

mauriciopasquier
Copy link
Contributor

@mauriciopasquier mauriciopasquier commented Aug 23, 2018

I wanted to have a default installation (i.e. with comments enabled) translated by default, so I added what was missing.

This repetition:

    comment: "Comment"
    active_admin/comment: "Comment"

is needed because comments_registration_name is Comment so it looks it up without namespace for title and menu translation. The rest is used by filters and scopes generated in the index.

I added hints for i18n-tasks about those ActiveRecord-related strings in the comments file so it can report them as missing when using i18n-tasks missing.

@codecov
Copy link

codecov bot commented Aug 24, 2018

Codecov Report

Merging #5458 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #5458   +/-   ##
=======================================
  Coverage   98.34%   98.34%           
=======================================
  Files         294      294           
  Lines       11050    11050           
=======================================
  Hits        10867    10867           
  Misses        183      183
Impacted Files Coverage Δ
lib/active_admin/orm/active_record/comments.rb 90.19% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef90f33...75314eb. Read the comment docs.

@mauriciopasquier
Copy link
Contributor Author

Sorry, I fixed-up and rebased, I had missed some errors before.

Copy link
Contributor

@varyonic varyonic left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

Looks great to me too! There's a couple of style things in the PR that we could try to enforce in the future, but we can think about it... in the future. Namely:

  • Always keep quoted values in our YAML files. Not very important, but would be nice.
  • Make sure we update the i18n hints list when/if new columns are added to the comments table. This happens very rarely, so it does not seem like a big deal.

@mauriciopasquier
Copy link
Contributor Author

Always keep quoted values in our YAML files. Not very important, but would be nice.

I noticed that style, that's why I changed active_admin.dashboard although it wasn't related to the commit per se. But I'm not sure how to enforce this at code level.

Make sure we update the i18n hints list when/if new columns are added to the comments table. This happens very rarely, so it does not seem like a big deal.

I was hesitant to introduce hints for that reason but I think the pros are enough :)

@deivid-rodriguez
Copy link
Member

Thanks so much @mauriciopasquier! ❤️

@deivid-rodriguez deivid-rodriguez merged commit 01acaf5 into activeadmin:master Aug 25, 2018
@mauriciopasquier mauriciopasquier deleted the comments-i18n branch August 25, 2018 21:40
@mauriciopasquier
Copy link
Contributor Author

Thank you all for this gem :)

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

3 participants