No chameleon; clearer Text, Heading, Block, and Box components #70
Conversation
|
FYI I haven't updated the docs on this branch because there aren't any relevant changes. The tests pass, so the output should be exactly the same. |
| return values.filter((v, i) => values.indexOf(v) === i) | ||
| } | ||
|
|
||
| function getBorderClass(value) { |
There was a problem hiding this comment.
This is maybe one for another PR, but what do you think about separating the border utilities into two different props? We could have border and borderColor? I think that would make reasoning with the prop a bit more intuitive on the user side & would make it easier to create classes here! It looks like the styleguide docs also encourage using two separate classes for border/border-top/etc and border color utilities
There was a problem hiding this comment.
Also curious if we need to allow people to pass in border={0}, and we instead make the Block component have no border by default?
| round, | ||
| shadow, | ||
| ...rest | ||
| } = map(props) |
There was a problem hiding this comment.
Can we rename map to something like mapUtilityProps?
| fg && `text-${fg}`, | ||
| position && `position-${position}`, | ||
| (typeof round === 'number') && `rounded-${round}`, | ||
| shadow && ((shadow === true) ? 'box-shadow' : `box-shadow-${shadow}`) |
There was a problem hiding this comment.
What do you think about changing the shadow API here to accept small, medium, large, extra-large and then instead of accepting either a boolean or a string we can just accept strings and this line would be
shadow && ((shadow === 'small') ? 'box-shadow' : box-shadow-${shadow}`)?
| ...rest | ||
| } = map(props) | ||
|
|
||
| const {style} = stylize(rest) |
There was a problem hiding this comment.
Is stylize here turning any extra props into inline styles? Curious what situations we're expecting to need this for?
There was a problem hiding this comment.
No, it's only passing along these props to style.
| nowrap: 'no-wrap' | ||
| }) | ||
| const fontSizeMap = { | ||
| 0: 6, |
There was a problem hiding this comment.
I know this wasn't introduced in this PR but curious why we are inverting the fontSize scale here?
|
@emplums thanks for all of that feedback! Let's file it in an issue for follow-up and merge this as-is, so that props API changes can be implemented and tested separately. ✌️ |
At @emplums suggestion, I've refactored the Text, Heading, Block, and Box components to be much clearer and removed the
chameleon()function, which was causing some confusion. I've also removed theclassifier(),valueMapper(), andexpander()functions fromprops.js, but keptstylizer()around for Block (and, I predict, other components).Team, I'd love all your eyes on this if you have time to take a look! ❤️