Fix multiple @font-face of the same name#327
Conversation
| import OrderedElements from './ordered-elements'; | ||
| import {generateCSS} from './generate'; | ||
| import {hashObject, hashString} from './util'; | ||
| import { generateCSS } from './generate'; |
There was a problem hiding this comment.
My editor auto formats this. I can change back if need be.
There was a problem hiding this comment.
I prefer it the way you have formatted it here, but Khan's style guide is the way it was. I don't think it matters too much, but these stylistic changes should probably be avoided to reduce diff churn. If we want to change the code style of this project, that should all be done at one time. Can you change this (and any other changes like this e.g. line 51) back?
| fontFamily: function fontFamily(val) { | ||
| if (Array.isArray(val)) { | ||
| return val.map(fontFamily).join(","); | ||
| const nameMap = {}; |
There was a problem hiding this comment.
Was gonna use Set, but I didn't see that or Map used anywhere else. Playing it safe.
There was a problem hiding this comment.
That's right, we've avoided these things so it would be nice to keep it that way unless there is a compelling reason not to.
I think the order of the keys in the object are not guaranteed on older versions of Node, but that might not matter anymore so this should be okay.
lencioni
left a comment
lencioni
left a comment
There was a problem hiding this comment.
Thanks for fixing this! I agree that Jest snapshots would be perfect for this and am in favor of switching to that. If you have time would you be willing to put up a PR that gets aphrodite onto Jest?
| import OrderedElements from './ordered-elements'; | ||
| import {generateCSS} from './generate'; | ||
| import {hashObject, hashString} from './util'; | ||
| import { generateCSS } from './generate'; |
There was a problem hiding this comment.
I prefer it the way you have formatted it here, but Khan's style guide is the way it was. I don't think it matters too much, but these stylistic changes should probably be avoided to reduce diff churn. If we want to change the code style of this project, that should all be done at one time. Can you change this (and any other changes like this e.g. line 51) back?
| fontFamily: function fontFamily(val) { | ||
| if (Array.isArray(val)) { | ||
| return val.map(fontFamily).join(","); | ||
| const nameMap = {}; |
There was a problem hiding this comment.
That's right, we've avoided these things so it would be nice to keep it that way unless there is a compelling reason not to.
I think the order of the keys in the object are not guaranteed on older versions of Node, but that might not matter anymore so this should be okay.
|
@lencioni Fixed formatting/spacing. |
ghost
mentioned this pull request
3 participants
Fix for #296. Verified the fix locally in one of my projects by editing the node_modules file.
Sidenote, I'd highly suggest using Jest snapshots to verify buffered output instead of
assertStylesInclude, which is extremely flakey.