Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix fontFamily being set to undefined #252

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dwayne-roberts
Copy link

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
Copy link

khanbot commented Jun 30, 2017

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
Copy link
Author

[clabot:check]

@khanbot
Copy link

khanbot commented Jul 3, 2017

CLA signature looks good 👍

Copy link
Contributor

@xymostech xymostech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

if (val instanceof OrderedElements) {
// We need to generate a unique key by which we can identify
// this declaration in the the 'alreadyInjected' object
key = `fontface_${hashObject(val)}`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

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

@dwayne-roberts
Copy link
Author

@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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants