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

Sign_in form fix #40

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Sign_in form fix #40

wants to merge 3 commits into from

Conversation

jroers
Copy link

@jroers jroers commented Feb 5, 2016

Fixes #7
Changed the placeholder values to be strings, No need for clownhats for 'Remember Me'

@tgaff
Copy link
Member

tgaff commented Feb 5, 2016

That might work, but we support several different languages in publify. We need to continue to support them. Please use the translations.

@tgaff
Copy link
Member

tgaff commented Feb 6, 2016

I think we already have strings in the locales.

@jroers
Copy link
Author

jroers commented Feb 6, 2016

Fixes #7
About this line:
<%= f.check_box :remember_me %> <%= "#{t('admin.users.remember_me', :default => "Remember Me")}" %>

There is not a field for remember_me that could I could find in any of the language files. Instead, I opted to stick with the same format that the other fields were using. I WOULD have used t('accounts.confirm.login') and t('accounts.confirm.password'); however, some (but not all) of the language files were looking to render additional information like %{login}.

For the sake of consistency across languages, I opted for the path admin.users.INSERT_THING_HERE

@nathanallen
Copy link
Contributor

Are there .yml file changes that you forgot to push?

@jroers
Copy link
Author

jroers commented Feb 6, 2016

I'm about 99% sure I didn't change any .yml files.

I just searched through them to find translations that corresponded to "Login" and "Password", then compared them with a few other languages to get a handle of whether they were being used consistently. Since they were, I didn't see the need to add new fields, or modify any of them as I was concerned that this could break the consistency.

@nathanallen
Copy link
Contributor

Nice work! I'd say we're two-thirds of the way there, and as far as we're going to get in this simulation without additional translations. You've uncovered two existing translations, and made a good argument for where the third one should go. As your code stops unsighly raw-html from displaying in the form, I am willing to merge this code -- but we'll need to follow up and create a new PR for "remember me" to be translated into other languages.

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

Successfully merging this pull request may close these issues.

3 participants