fix: fontWeight and fullWidth on marketplace screen#1474
Conversation
Signed-off-by: clegirar <clemntgirard@gmail.com>
✅ Deploy Preview for teritori-dapp ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for gno-dapp ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Please fix the // FIXME: remove StyleSheet.create when you work on a file that mentions that :)
There was a problem hiding this comment.
I think do some PR's that fix that should be better
There was a problem hiding this comment.
Please fix the // FIXME: remove StyleSheet.create
| marginRight: layout.spacing_x0_5, | ||
| }, | ||
| fontRegular14, | ||
| { lineHeight: 19, marginRight: layout.spacing_x0_5 }, |
There was a problem hiding this comment.
Until we don't need an extra height for a text, the lineHeight must be defined by fontRegular14 here.
We should remove a maximum of useless styles
| { lineHeight: 19, marginRight: layout.spacing_x0_5 }, | |
| {marginRight: layout.spacing_x0_5 }, |
Line height 19px that doesn't make sense because there is still a parent (row) with alignItems.
What issue could we have with an extra lineHeight ?
It can "shift" the aligned Items.
Here, it adds like 1px under "26.84K" (yea, it's a detail)
No line height 19px, normal align items
There was a problem hiding this comment.
fontRegular14 defined a line height (18).
Agree with the fact that define like that in the style a lineHeight doesn't make sense, and it should be cool to have the exact style (lineHeight included) for every fontRegular14 we use.
But i think we're in agreement 👍
There was a problem hiding this comment.
fontRegular14 has line height 16.
And I didn't understand if you will remove lineHeight: 19
But it's too detail, I'll LGTM :)
packages/components/nfts/NFTView.tsx
Outdated
| fontSize: 14, | ||
| marginBottom: 12, | ||
| }} | ||
| style={[fontRegular14, { fontSize: 14, marginBottom: 12 }]} |
There was a problem hiding this comment.
| style={[fontRegular14, { fontSize: 14, marginBottom: 12 }]} | |
| style={[fontRegular14, { marginBottom: 12 }]} |
packages/components/nfts/NFTView.tsx
Outdated
| lineHeight: 16, | ||
| }} | ||
| > | ||
| <BrandText style={[fontRegular12, { lineHeight: 16 }]}> |
There was a problem hiding this comment.
| <BrandText style={[fontRegular12, { lineHeight: 16 }]}> | |
| <BrandText style={fontRegular12}> |
Signed-off-by: clegirar <clemntgirard@gmail.com>
| marginRight: layout.spacing_x0_5, | ||
| }, | ||
| fontRegular14, | ||
| { lineHeight: 19, marginRight: layout.spacing_x0_5 }, |
There was a problem hiding this comment.
fontRegular14 has line height 16.
And I didn't understand if you will remove lineHeight: 19
But it's too detail, I'll LGTM :)
|
I checked the result of the text and it changed nothing, so I just wanted to stay consistent 👍 |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
3 participants
Change
fontWeightandfullWidthon marketplace screen.Before:
After:
