Fix fontFamily being set to undefined

[dwayne-roberts]

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.

[khanbot]

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

[dwayne-roberts]

[clabot:check]

[khanbot]

CLA signature looks good 👍

[xymostech]

Hey @dwayne-roberts! Thanks for the PR.

It looks like the travis coverage tests are failing because you didn't add a test to check that these two lines work. Can you add those? Other than that, this looks great!

[xymostech]

Is there a reason we're not just using val.get('src') to be consistent with how we generate the key normally?

[dwayne-roberts]

Sorry for the late reply, was away. No, there isn't a reason I didn't use val.get(). Good point. I'll change it to be consistent like you said. Thanks. And I'll also add the tests.

[xymostech]

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

[dwayne-roberts]

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

[xymostech]

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