Skip to content

Conversation

@ryandrewjohnson
Copy link
Owner

@ryandrewjohnson ryandrewjohnson commented Jul 13, 2018

Credit to @colbyfayock for the original fix included in this PR 🎉

Fix for #101

Two issues:

The application doesn't take the first item as a default language when a defaultLanguage isn't present
When falling back to the default translation using onMissingTranslation, the translation doesn't run through getLocalizedElement which prevents any variables from getting parsed
Solution

defaultLanguage

The getOptions function now falls back to the first available language if a defaultLanguage isn't provided.

onMissingTranslation

The logic for onMissingTranslation has been moved out of getLocalizedElement and into getTranslate, which makes more sense as getLocalizedElement doesn't care whether translations are missing or not - It just wants a string to localize.

@codecov
Copy link

codecov bot commented Jul 13, 2018

Codecov Report

Merging #110 into master will increase coverage by 0.08%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #110      +/-   ##
=========================================
+ Coverage   95.51%   95.6%   +0.08%     
=========================================
  Files           6       6              
  Lines         245     250       +5     
  Branches       73      77       +4     
=========================================
+ Hits          234     239       +5     
  Misses         10      10              
  Partials        1       1
Impacted Files Coverage Δ
src/Translate.js 100% <ø> (ø) ⬆️
src/utils.js 100% <100%> (ø) ⬆️
src/localize.js 96.8% <100%> (+0.25%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4f045e3...34034e7. Read the comment docs.

@ryandrewjohnson
Copy link
Owner Author

@colbyfayock I've had to create a new PR as I don't think I have access to push to the branch your work was in. I've included your original changes in this PR, but made some changes as well. After some thought I realized a lot of the complications were because onMissingTranslation was being passed as a param to getLocalizedElement.

This issue made it clear that there really is no reason for getLocalizedElement to handle the missing translation stuff. I have moved the missing translation logic directly into getTranslate, which greatly simplified things, and avoided the whole circular dependency issue that was happening when trying to run getLocalizedElement on defaultTranslation.

In addition I have taken care of all the flow issues. When you get a chance if you could have a look, and see if this still solves all the issues you were having. It passes the initial tests you wrote so I think we're good, but want to double check.

@colbyfayock
Copy link

@ryandrewjohnson that works, np. thanks for including the work :)

that all looks like it makes sense - and if the tests pass 👍i won't have a chance to actually pull that into the project i'm working with until next week, but the tests I set up should cover the same use cases I was running into issues with

thanks for looking at that

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants