getgrav / grav-plugin-login

Grav Login Plugin
http://getgrav.org
MIT License
44 stars 54 forks source link

Missing/fixing language strings #254

Closed juliendude closed 4 years ago

juliendude commented 4 years ago

Missing lang string for placeholder, button and messages.

I hope it helps. Don't hesitate if you have any question or if I can help some more the great grav project.

Since I am not sure the reasoning behind it, I did not add label in the field attribute on some pages (e.g.: reset.md). See bug #253

Vivalldi commented 4 years ago

Looks good so far. Since we're remapping BTN_RESET to BTN_RESET_PASSWORD (due to addition of plain BTN_RESET the already existing language files need to reflect this change as well

juliendude commented 4 years ago

Hi @Vivalldi I'm not sure I follow you with "existing language files need to reflect this change as well" ? Do you mean that I missed something? BTN_RESET_PASSWORD is on line 19 in reset-form.html.twig. Is that what you meant?

Vivalldi commented 4 years ago

Hi!

The en.yaml (and fr.yaml) changed

BTN_RESET: Reset Password

to

BTN_RESET: Reset
BTN_RESET_PASSWORD: Reset Password

es.yaml (and other languages) were not changed. Meaning that those languages will fallback to en for BTN_RESET_PASSWORD.

There's also the case where now in some languages BTN_RESET can either mean password reset or just reset

I haven't had time to review the rest of the PR but I'm sure the Andy or Matias will chime in.


p.s. Welcome to the Grav community! I see you're a first-time contributor. Glad to have you onboard 🎉

juliendude commented 4 years ago

Good point on other language files !

Since I don't speak these languages, do you suggest that I still add the strings with the English text? I'm all ears. Let know the way it gets done in the Grav development.

I also wanted to order the strings by alphabetical order to clean this up a bit, but I don't want to impose.

ps: thank you for your welcoming words. I really enjoy Grav. It's actually fun to develop with.

ricardo118 commented 4 years ago

you could order the strings alphabetically but usually they might be organized in sections of content, regarding the text in english since we dont want the fallback to happen then yes we would need english across all languages manually.

Another big + to the community, join the Discord the core devs and the community is active and helpful

Vivalldi commented 4 years ago

So what I would do is rename BTN_RESET to BTN_RESET_PASSWORD in the other languages as those strings shouldn't change. And then you can just leave the BTN_RESET empty or go with the English strings as @ricardo118 mentioned

rhukster commented 4 years ago

This looks good to me, is it ready to merge???

juliendude commented 4 years ago

Thank you @ricardo118 and @Vivalldi, I'll push these changes.

@rhukster Sorry for my late reply, just got back from a long week-end out of the city. I'll push the latest changes tomorrow (Monday) the latest and update the PR. Thank you for waiting.

juliendude commented 4 years ago

@rhukster as promise, here is the new PR where I applied the same changes and new strings to the rest of the language files. FYI in case you are not aware. Some language files are missing a lot of string found in the English file. Unfortunately, I do not speak these languages. I can only help with English and French.

rhukster commented 4 years ago

thanks so much! merged...