Skip to content
This repository has been archived by the owner on Jan 9, 2023. It is now read-only.

ember-intl implementation fixes #1562

Merged

Conversation

chadian
Copy link
Contributor

@chadian chadian commented Nov 17, 2018

Partially fixes #1561.

  • remove empty string translations
  • setting a missing translation message to an empty string
  • Adding the fallback locale

cc @HospitalRun/core-maintainers

@chadian chadian force-pushed the ember-intl-implementation-fixes branch from 61a64b5 to 30e9f59 Compare November 17, 2018 09:56
@chadian chadian force-pushed the ember-intl-implementation-fixes branch from 30e9f59 to 7ccc9f4 Compare November 17, 2018 10:55
@chadian chadian mentioned this pull request Nov 17, 2018
7 tasks
- Change the css implmentation to use the root body direction attribute.
- Use rtl-detect instead of managing rtl configuration
@chadian chadian force-pushed the ember-intl-implementation-fixes branch from aa8cae7 to fa16f3a Compare November 17, 2018 21:12
@MatthewDorner
Copy link
Contributor

Took a look at the RTL with Urdu language, looks good except I noticed something else: the font changes. It even sometimes changes in the middle of a translation item, and there's no HTML or anything in the translation string, I wonder what it is. And I don't see this in any other locale even other RTL ones.

rtl urdu

Also noticed some additional issues that were also present before the switch to intl so I can make a new issue for those later.

@MatthewDorner
Copy link
Contributor

And, checked and that font thing happened before the RTL changes, but it is somehow related to switching to ember-intl. We can just put it with the other things to check out later.

Everything else looks good, are you still working on anything?

@chadian
Copy link
Contributor Author

chadian commented Nov 18, 2018

That's really interesting actually. I can't explain the font change for urdu. The characters look like they could be the same, I'm not sure if the change is acceptable or not either. I agree, let's do this as a separate issue and I'm happy to also investigate. Nothing else for this PR from my end, thanks! 👍

@MatthewDorner
Copy link
Contributor

The characters look to be identical, just different font so I doubt it's a big deal. Thanks for the quick fixes.

@MatthewDorner
Copy link
Contributor

Actually one more thing occurs to me, in #1458 we set all those translation keys to undefined instead of removing them so people could more easily see in the file what keys need to be translated.

Would it be better to comment them out instead of removing them entirely?

@MatthewDorner
Copy link
Contributor

Maybe that's not a good idea either since if people add keys, they'll just forget to add commented out keys in the other languages so they still won't stay synced up.

@MatthewDorner MatthewDorner merged commit 856bcb2 into HospitalRun:master Nov 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ember-intl upgrade side effects
2 participants