aphrodite
aphrodite copied to clipboard
Fix fontFamily being set to undefined
Added a check for an instanceof
OrderedElement
for the fontFamily
StringHandler
helper function.
There was a check if the value passed was an Array, Object or String. When it was an Object it keys were accessed directly. But Objects of type OrderedElement
have a different shape. I added a check if it was of type OrderedElement
and so go about using it's getter function OrderedElement.get()
otherwise just on the object directly.
Hey @dwayne-roberts,
Thanks for the PR! Mind signing our Contributor License Agreement?
When you've done so, go ahead and comment [clabot:check]
and I'll check again.
Yours truly, khanbot
[clabot:check]
CLA signature looks good :+1:
@dwayne-roberts ping on this? Would love to get this merged :)
@xymostech could you point me in the right direction for the test? Unfortunately I have little experience with these type of test. I only ever used Jest and snapshot tests. :-(
The coverage tests make sure that all branches in the code are tested. We do that by running the tests and then seeing all of the lines that they hit. In this case, there's a line in the failing coverage tests that says
----------------------|----------|----------|----------|----------|----------------|
File | % Stmts | % Branch | % Funcs | % Lines |Uncovered Lines |
----------------------|----------|----------|----------|----------|----------------|
...
inject.js | 97.89 | 97.56 | 100 | 97.89 | 75,76 |
which means that lines 75 and 76 of src/inject.js aren't getting hit during the tests. Looking at the code, it's just the new branch that you added in, which indicates that there isn't a test for the new code that you wrote.
So, writing a test here: https://github.com/Khan/aphrodite/blob/master/tests/inject_test.js#L333 that hits this change would probably fix this. You can run npm run test
to make sure that it's fixed.
Hopefully that should help! Let me know if anything is still confusing. :)