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

Extraction of i18n strings in system panel #11555

Merged
merged 25 commits into from Sep 22, 2023

Conversation

greenwoodt
Copy link
Contributor

@greenwoodt greenwoodt commented Aug 31, 2023

🎩 What? Why?

Extracting and adding i18n translations to system docs where needed, making the use of strings not needed.

📌 Related Issues

Link your PR to an issue

Testing

📷 Screenshots

♥️ Thank you!

@alecslupu
Copy link
Contributor

@greenwoodt this pr is not complete, as there are more strings to be extracted. Please see all the comments in the ticket

@greenwoodt
Copy link
Contributor Author

@greenwoodt this pr is not complete, as there are more strings to be extracted. Please see all the comments in the ticket

I'm fully aware @alecslupu thanks. I'll notify you for a review once it is.

@greenwoodt greenwoodt marked this pull request as draft September 12, 2023 11:15
@greenwoodt greenwoodt marked this pull request as ready for review September 15, 2023 12:41
@greenwoodt
Copy link
Contributor Author

greenwoodt commented Sep 15, 2023

@alecslupu I have changed and added translations according to your request on issue: #11481, however there was one I was stuck with on adding as the string does interpolate a class variable, located here: decidim-system/app/views/decidim/system/devise/shared/_links.html.erbon line 23.

I wondered whether you could provide insight on changing this? Does one at the interploation on the locales.en file directly and just call the translation?

@greenwoodt
Copy link
Contributor Author

I have reverted line 12 back to Decidim here: decidim-system/app/views/layouts/decidim/system/login.html.erbas it was failing tests to do with yield and not being able to find the translation.

@andreslucena
Copy link
Member

@alecslupu I have changed and added translations according to your request on issue: #11481, however there was one I was stuck with on adding as the string does interpolate a class variable, located here: decidim-system/app/views/decidim/system/devise/shared/_links.html.erbon line 23.

I wondered whether you could provide insight on changing this? Does one at the interploation on the locales.en file directly and just call the translation?

Check the Rails docs section. It's like this:

 t(".sign_in_with", provider: OmniAuth::Utils.camelize(provider))

And then in the relevant en.yml section:

sign_in_with: Sign in with %{provider}

But giving a second look to the feature itself, I'd said that we should remove this altogether, as:

  1. In the context of Decidim, I can't imagine an instance that would want to enable omniauth for the system panel
  2. Even if it does, this would never work, as it's referring to the current_organization helper that is not available in /system

So, its safe to remove this whole block:

 <%- if devise_mapping.omniauthable? %>
   <%- current_organization.enabled_omniauth_providers.keys.each do |provider| %>
     <%= link_to "Sign in with #{OmniAuth::Utils.camelize(provider)}", omniauth_authorize_path(resource_name, provider) %>
   <% end -%>
 <% end -%>

Can you do that @greenwoodt ? Thanks

@andreslucena
Copy link
Member

I have reverted line 12 back to Decidim here: decidim-system/app/views/layouts/decidim/system/login.html.erbas it was failing tests to do with yield and not being able to find the translation.

What if we add the string that's missing in the en.yml file? I'm referring to this one:

Translation missing: en.layouts.decidim.shared.layout_center.decidim

@greenwoodt
Copy link
Contributor Author

I have reverted line 12 back to Decidim here: decidim-system/app/views/layouts/decidim/system/login.html.erbas it was failing tests to do with yield and not being able to find the translation.

What if we add the string that's missing in the en.yml file? I'm referring to this one:

Translation missing: en.layouts.decidim.shared.layout_center.decidim

The file still fails a test, even after adding the string manually. I can try again today and let you know.

@andreslucena andreslucena changed the title Extraction of i18n strings Extraction of i18n strings in system panel Sep 18, 2023
@andreslucena andreslucena added the type: fix PRs that implement a fix for a bug label Sep 18, 2023
Copy link
Contributor

@alecslupu alecslupu left a comment

Choose a reason for hiding this comment

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

There is one key that should not be as it is.

decidim-system/config/locales/en.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@alecslupu alecslupu left a comment

Choose a reason for hiding this comment

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

@greenwoodt , please check also : decidim-system/app/views/decidim/system/devise/shared/_links.html.erb Line 2 ( there is a "Log in" left befind)

#11478 (comment)

greenwoodt and others added 5 commits September 19, 2023 09:08
Co-authored-by: Alexandru Emil Lupu <contact@alecslupu.ro>
…ml.erb

Co-authored-by: Alexandru Emil Lupu <contact@alecslupu.ro>
…html.erb

Co-authored-by: Alexandru Emil Lupu <contact@alecslupu.ro>
Co-authored-by: Alexandru Emil Lupu <contact@alecslupu.ro>
@greenwoodt

This comment was marked as resolved.

Copy link
Member

@andreslucena andreslucena left a comment

Choose a reason for hiding this comment

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

I've just checked out all the comments from the original issue and I found a missing one:
#11478 (comment)

Regarding the rest, codewise is 🆗

Copy link
Member

@andreslucena andreslucena left a comment

Choose a reason for hiding this comment

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

Sorry, wrong button 😅
Can you check my last review? 🙏🏽

@greenwoodt
Copy link
Contributor Author

greenwoodt commented Sep 21, 2023

Sorry, wrong button 😅 Can you check my last review? 🙏🏽

No worries! Check my comment here regarding this change that didn't work. I left it out purposefully as it was causing errors to the tests.

@alecslupu
Copy link
Contributor

alecslupu commented Sep 21, 2023

Sorry, wrong button 😅 Can you check my last review? 🙏🏽

No worries! Check my comment here regard this change that didn't work. I left it out purposefully as it was causing errors to the tests.

I have checked 66fc58d, and it seems that you have forgot to add it to en.yml file. it should work without any kind of issues, once you add layouts.decidim.shared.layout_center.decidim key to the en.yml.

As as alternative you could define it as decidim in the scope of decidim.system.titles like this: <%= t("decidim", scope: "en.decidim.system.titles") %>

@alecslupu alecslupu added this to the 0.28.0 milestone Sep 21, 2023
@greenwoodt
Copy link
Contributor Author

@alecslupu @andreslucena updated and ready.

Copy link
Contributor

@alecslupu alecslupu left a comment

Choose a reason for hiding this comment

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

LGTM

@alecslupu alecslupu dismissed andreslucena’s stale review September 22, 2023 08:57

The PR is fairly simple, and I already reviewed it.

@alecslupu alecslupu merged commit 57827f4 into decidim:develop Sep 22, 2023
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: system target:cleanup type: fix PRs that implement a fix for a bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Extract I18n strings from templates
3 participants