aphrodite icon indicating copy to clipboard operation
aphrodite copied to clipboard

Fix fontFamily being set to undefined

Open dwayne-roberts opened this issue 7 years ago • 6 comments

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.

dwayne-roberts avatar Jun 30 '17 13:06 dwayne-roberts

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

khanbot avatar Jun 30 '17 13:06 khanbot

[clabot:check]

dwayne-roberts avatar Jul 03 '17 08:07 dwayne-roberts

CLA signature looks good :+1:

khanbot avatar Jul 03 '17 08:07 khanbot

@dwayne-roberts ping on this? Would love to get this merged :)

xymostech avatar Jul 12 '17 21:07 xymostech

@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. :-(

dwayne-roberts avatar Jul 24 '17 16:07 dwayne-roberts

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. :)

xymostech avatar Jul 24 '17 18:07 xymostech