Refactoring text component #769
Conversation
|
Awesome Storybook work 👍 just want to make sure there will be no breaking changes with this refactor? |
| ` | ||
| display: 'inline-block'; | ||
| `} | ||
|
|
||
| ${props => | ||
| props.txtcenter && | ||
| ` | ||
| textAlign: 'center'; | ||
| `} | ||
|
|
||
| ${props => | ||
| props.txtRight && | ||
| ` | ||
| textAlign: 'right'; | ||
| `} | ||
|
|
||
| ${props => | ||
| props.superSmall && | ||
| ` | ||
| font-size: ${theme.fontSizes[0]}; | ||
| `} | ||
|
|
||
| ${props => | ||
| props.small && | ||
| ` | ||
| font-size: ${theme.fontSizes[1]}; | ||
| `} | ||
|
|
||
| ${props => | ||
| props.medium && | ||
| ` | ||
| font-size: ${theme.fontSizes[2]}; | ||
| `} | ||
|
|
||
| ${props => | ||
| props.large && | ||
| ` | ||
| font-size: ${theme.fontSizes[3]}; | ||
| `} | ||
|
|
||
| ${props => | ||
| props.regular && | ||
| ` | ||
| font-weight: 400; | ||
| `} | ||
|
|
||
| ${props => | ||
| props.bold && | ||
| ` | ||
| font-weight: 600; | ||
| `} | ||
|
|
||
| ${props => | ||
| props.uppercase && | ||
| ` | ||
| text-transform: uppercase; | ||
| `} | ||
|
|
||
| ${props => | ||
| props.capitalize && | ||
| ` | ||
| text-transform: capitalize; | ||
| `} | ||
|
|
||
| ${props => | ||
| props.tags && | ||
| ` | ||
| font-size: 12px; | ||
| color: ${theme.colors.blue}; | ||
| `} | ||
|
|
||
| ${props => | ||
| props.auxiliary && | ||
| ` | ||
| font-family: "Inter", Helvetica Neue, Arial, sans-serif; | ||
| font-size: 12px; | ||
| color: #686868; | ||
| `} | ||
|
|
||
| ${props => | ||
| props.paragraph && | ||
| ` | ||
| font-family: "Inter", Helvetica Neue, Arial, sans-serif; | ||
| font-size: 16px; | ||
| color: ${theme.colors.grey}; | ||
| `} | ||
|
|
||
| ${props => | ||
| props.clipped && | ||
| ` | ||
| white-space: nowrap; | ||
| text-overflow: ellipsis; | ||
| overflow: hidden; | ||
| `} | ||
|
|
||
| ${props => | ||
| props.preLine && | ||
| ` | ||
| white-space: pre-line; | ||
| `} |
There was a problem hiding this comment.
Yep that's more clear thanks. I was wondering though if you had an idea to reduce the number of possible props to the text component ?
There was a problem hiding this comment.
What we have at the moment:
inline?: boolean
regular?: boolean
txtcenter?: boolean
capitalize?: boolean
bold?: boolean
txtRight?: boolean
large?: boolean
medium?: boolean
small?: boolean
superSmall?: boolean
color?: keyof typeof theme.colors
clipped?: boolean
preLine?: boolean
tags?: boolean
auxiliary?: boolean
paragraph?: boolean
What I think we can reduce it to:
color?: keyof typeof theme.colors
large?: boolean
medium?: boolean
small?: boolean
superSmall?: boolean
align?: string
bold?: boolean
uppercase?: boolean
capitalize?: boolean
clipped?: boolean
Plus, I would have the following separate components for the predefined styles:
ParagraphText
AuxiliaryText
TagsText
Those can simply be export default styled(Text)`color: blah; font-size: bleh;` exported as a different component.
Then we have left inline?: boolean which maybe every Text could be by default, and if needed to be a block element, just wrap it in a div.
What do you think?
There was a problem hiding this comment.
Yes props make sense 👍
I like the separate component for ParaphText, AuxiliaryText, TagsText.
How I would see it is in the src/components/Text/index.ts having the Text component exported by default and then have exported const ParaphText, AuxiliaryText, TagsText.
There was a problem hiding this comment.
i will go ahead with that
|
@drydenwilliams So far this text refactoring would have no breaking change I assume, since the interface of the component (props, etc) haven't changed. Once I reply to @BenGamma about minimizing the amount of props, that might be something we need to go back and fix all uses of the component.
|
Yes that would require a little bit of refactoring and UI check, but should be ok 🙆♂ |
caiokf
force-pushed
the
refactoring-text-component
branch
from
b11307a to
9d15560
Compare
|
Test summary
Run details
View run in Cypress Dashboard ➡️ This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
|
@caiokf I pushed some minor changes to be able to compile on |
|
As this is now quite old (not sure whether overlooked, in which case apologies, or there were issues) and has conflicts I'm going to close here, but feel free to re-open if still relevant and resolved |
Dependent on #743, please merge that one first.