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

Internationalize/I18n the Dashboard Engine #497

Merged

Conversation

JuanVqz
Copy link
Contributor

@JuanVqz JuanVqz commented Jan 26, 2022

@bensheldon, I had a little bit of progress, so I would like to share it with you

Related #408

Links:
ruby on rails docs

@JuanVqz JuanVqz force-pushed the internationalize-the-dashboard-engine branch 2 times, most recently from fbe0144 to 0ed18d9 Compare January 27, 2022 00:41
@JuanVqz
Copy link
Contributor Author

JuanVqz commented Jan 27, 2022

@bensheldon what do you think about the variable's name in the locales/en.yml file?

Is there a convention? Do you know?

@bensheldon
Copy link
Owner

@JuanVqz That looks fantastic! You did what I was imagining: Used relative keys (e.g. .key) that are namespaced to the template 👍

For the key names themselves, generic naming is ideal that describes the purpose of the string, rather than the contents. E.g.

  • "timezone_notice"
  • "current_time"
  • "coming_soon"

@JuanVqz JuanVqz force-pushed the internationalize-the-dashboard-engine branch from 0ed18d9 to 7208bca Compare January 27, 2022 03:25
@bensheldon, I had a little bit of progress, so I would like to share it with you
@JuanVqz JuanVqz force-pushed the internationalize-the-dashboard-engine branch from 7208bca to cd6539c Compare January 27, 2022 04:00
@JuanVqz
Copy link
Contributor Author

JuanVqz commented Jan 27, 2022

I have already fixed the test that struggled in the CI.

@bensheldon
Copy link
Owner

This looks great. I think this would be a good time for you to pause on extracting strings, and next look into UI and routing to display the different locales.

@JuanVqz
Copy link
Contributor Author

JuanVqz commented Jan 27, 2022

This looks great. I think this would be a good time for you to pause on extracting strings, and next look into UI and routing to display the different locales.

Yes, I agree, it seems a good time to pause it. What do you mean by looking into IU, should I add a select field with available languages?

Also, I have a question, until now I have been seen the /good_job page though the rspec test when running the system spec, but I'm wondering how can I see it in a rails app, I saw in the read me that I should point to the local gem but I don't fully understand how can I do it.

Should I create a new rails new app_name or the one is already in the rspec folder?
bundle config local.good_job /path/to/local/git/repository

@bensheldon
Copy link
Owner

What do you mean by looking into UI, should I add a select field with available languages?

Yep. I think simplest would just be a drop down menu in the Navbar with simple links. The Bootstrap docs should show you how to do that.

Great question about running it. You can run GoodJob's dashboard directly from the good job repo:

bin/setup
bin/test_app s # runs rails server

@JuanVqz
Copy link
Contributor Author

JuanVqz commented Jan 30, 2022

Great question about running it. You can run GoodJob's dashboard directly from the good job repo:

bin/setup
bin/test_app s # runs rails server

this is super useful, thank you for sharing it.

I already started playing with the select field in the view, and I noticed that we may use the locale functionality in at least two ways, and I'm wondering which one would you like to have,

1 localhost:3000/en/good_job
I think, I will need to scope the routes in the route file

  # route.rb
  scope :lang do
    # routes
  end

2 localhost:3000/good_job?locale=en

Also, I'm wondering if the select field, should change the language once the user changes the option, or could we have a button to trigger the selection...

@bensheldon
Copy link
Owner

@JuanVqz oops, I didn't answer your question. I think option #2 is good for now.

This is the dropdown menu I think you should use, which would contain links: https://getbootstrap.com/docs/5.1/components/navs-tabs/#using-dropdowns

@JuanVqz
Copy link
Contributor Author

JuanVqz commented Mar 29, 2022

@JuanVqz oops, I didn't answer your question. I think option #2 is good for now.

This is the dropdown menu I think you should use, which would contain links: https://getbootstrap.com/docs/5.1/components/navs-tabs/#using-dropdowns

There are some failing tests but it seems like we have some progress here.

@bensheldon what do you think?

Peek 2022-03-28 23-06

@bensheldon
Copy link
Owner

@JuanVqz AWESOME! I will dive in later this week and get those tests passing. This looks really good.

@JuanVqz JuanVqz force-pushed the internationalize-the-dashboard-engine branch from b86bd5b to f64254c Compare March 31, 2022 01:14
@JuanVqz JuanVqz force-pushed the internationalize-the-dashboard-engine branch from f64254c to de6fccd Compare March 31, 2022 01:39
@JuanVqz
Copy link
Contributor Author

JuanVqz commented Mar 31, 2022

@JuanVqz AWESOME! I will dive in later this week and get those tests passing. This looks really good.

I got it green, there were some minor issues, also, I added the footer partial which didn't exist when I started working on this feature, now it seems ready to go...I'm open to making changes if needed. 👍

@@ -3,7 +3,7 @@ module GoodJob
class BaseController < ActionController::Base # rubocop:disable Rails/ApplicationController
protect_from_forgery with: :exception

before_action :set_locale
around_action :switch_locale
Copy link
Contributor Author

Choose a reason for hiding this comment

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

why do you prefer around_action over before_action?

def set_locale
I18n.locale = current_locale
def switch_locale(&action)
I18n.with_locale(current_locale, &action)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

what do you prefer with_locale?

Copy link
Owner

Choose a reason for hiding this comment

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

Good question. I like that with_locale only changes the locale within the context of the block and then returns to the previous value. Because the Goodjob engine is running in the context of a parent application, I don't want to change the I18n.locale value globally because it might have an unexpected side effect in the parent application.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good to know, yes, we don't want unexpected behavior 👍

Thank you for the info

@bensheldon
Copy link
Owner

@JuanVqz thank you! I feel great about this. Any items before I merge and release?

Thank you so much!!

@JuanVqz
Copy link
Contributor Author

JuanVqz commented Apr 2, 2022

@JuanVqz thank you! I feel great about this. Any items before I merge and release?

Thank you so much!!

I'm happy to be part of it, I don't think so, I believe it is ready to be merged!

Thank you for the opportunity to contribute, I'll be around if I can take another issue ⚠️

@bensheldon bensheldon merged commit ee8e1b4 into bensheldon:main Apr 5, 2022
@JuanVqz
Copy link
Contributor Author

JuanVqz commented Apr 5, 2022

❤️ thank you!!!

@JuanVqz JuanVqz deleted the internationalize-the-dashboard-engine branch April 5, 2022 18:16
@@ -24,10 +24,28 @@ class BaseController < ActionController::Base # rubocop:disable Rails/Applicatio
request.content_security_policy_nonce_generator = ->(_request) { SecureRandom.base64(16) }
end

def default_url_options(options = {})
Copy link

Choose a reason for hiding this comment

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

Is there a particular need for it to be public? Now it's treated as an action.

Copy link
Owner

Choose a reason for hiding this comment

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

@igas good catch. Could you make a PR for that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

3 participants