Move font-size and warn utils out of base, refactor conditionals, add bgImageStyle #542
Conversation
DDunc
force-pushed
the
feature/add-bg-image-style
branch
2 times, most recently
from
176bb87 to
ba85d3b
Compare
DDunc
force-pushed
the
feature/add-bg-image-style
branch
from
ba85d3b to
fa9fa4a
Compare
ryan-roemer
left a comment
ryan-roemer
left a comment
There was a problem hiding this comment.
Looks like a good start here. Tests are failing in CI, but assuming that's on the task list to fix too.
| export const buildStyles = (transforms, props, context, styles = {}) => { | ||
| return transforms.reduce((av, cv) => { | ||
| return { ...av, ...cv(props, context, av) }; | ||
| }, styles); |
There was a problem hiding this comment.
Did we want to mutate the styles object passed into us?
There was a problem hiding this comment.
Since we're using styles as an initial value and returning a new value, I don't think it would be expected behavior to have side effects.
src/utils/base.js
Outdated
| } | ||
| }; | ||
| export const transformBold = ({ bold }) => { | ||
| if (typeof italic === 'boolean') { |
There was a problem hiding this comment.
typeof italic? Looks like the wrong variable referenced.
Shouldn't lint catch this?
There was a problem hiding this comment.
I would certainly prefer it to, but eslint no-undef skips typeof by default. After declaring"no-undef": ["error", { "typeof": true }] in the eslintrc and running it against all the project files it doesn't look like we're using that typeof 'feature' anywhere else.
DDunc
changed the title
DDunc
force-pushed
the
feature/add-bg-image-style
branch
from
6e0d530 to
5fd5725
Compare
*bgImageStyle prop allows for setting the backgroundImage property directly (resolve #428)
*bgLighten prop allows for setting opacity with white instead black (based on #531, happy to rebase/refactor for the bgLighten warning)
*Refactor for greater purity/granularity/testability
*Disabled consistent-return for base.js. This seems like an Airbnb rule that's part of a larger discussion about expectations of implicit returns, but would add a lot of boilerplate here.
*Validated that old and new getStyles functions return the same objects/trigger the same one time warnings but some tests would still be a good idea.