-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: fontWeight and fullWidth for Wallet manager screen #1473
Conversation
Signed-off-by: clegirar <[email protected]>
✅ Deploy Preview for gno-dapp ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for teritori-dapp ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some changes that are not fully in scope, but are easy to fix and to review in this kind of "text refacto" PR:
- Remove StyleSheet usages
- Remove style array usage if not needed (if present in your changes) (Ex:
style={[fontMedium13]}
<BrandText style={[fontRegular12]}>{title}</BrandText> | ||
<BrandText style={[fontRegular16]}>{data}</BrandText> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a smaaaall detail, but we can remove the arrays usage is styles (I forgot too sometimes lol)
<BrandText style={[fontRegular12]}>{title}</BrandText> | |
<BrandText style={[fontRegular16]}>{data}</BrandText> | |
<BrandText style={fontRegular12}>{title}</BrandText> | |
<BrandText style={fontRegular16}>{data}</BrandText> |
@@ -83,7 +87,7 @@ export const DepositWithdrawModal: React.FC<DepositModalProps> = ({ | |||
<View style={styles.rowCenter}> | |||
<NetworkIcon networkId={networkId} size={32} /> | |||
<SpacerRow size={3} /> | |||
<BrandText> | |||
<BrandText style={[fontRegular16]}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<BrandText style={[fontRegular16]}> | |
<BrandText style={fontRegular16}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
StyleSheet usage in this file
@@ -112,21 +113,15 @@ export const WalletItem: React.FC<WalletItemProps> = ({ | |||
<WalletProviderIcon walletProvider={item.provider} size={64} /> | |||
<View style={{ marginLeft: 16 }}> | |||
<View> | |||
<BrandText>{item.provider}</BrandText> | |||
<BrandText style={[fontRegular18]}>{item.provider}</BrandText> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<BrandText style={[fontRegular18]}>{item.provider}</BrandText> | |
<BrandText style={fontRegular18}>{item.provider}</BrandText> |
> | ||
{item.address} | ||
</BrandText> | ||
<BrandText style={[fontRegular12]}>{item.address}</BrandText> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<BrandText style={[fontRegular12]}>{item.address}</BrandText> | |
<BrandText style={fontRegular12}>{item.address}</BrandText> |
fontSize: 14, | ||
}} | ||
> | ||
<BrandText style={[fontRegular14]}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<BrandText style={[fontRegular14]}> | |
<BrandText style={fontRegular14}> |
fontSize: 14, | ||
}} | ||
> | ||
<BrandText style={[fontRegular14]}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<BrandText style={[fontRegular14]}> | |
<BrandText style={fontRegular14}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove StyleSheet usages when you see that in your changed files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
StyleSheet usage in this file (pretty big)
Signed-off-by: clegirar <[email protected]>
Yeah it's out of scope, but we can do some PR to remove |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Fixed
fontWeight
andfullWidth
forWallet manager
screenIn packages/components/inputs/TextInputCustom.tsx i put a text color by default (i did the same thing than here BrandTextBase.tsx)
Before:
After:
I changed just the spacing of some style props which were many lines and could fit in one line (example here)
For the
fullWidth
screen, we are obligate to do that for the moment.But at the end, it would be great to move that in the future
ScreenContainer
refacto, and just passfullWidth
toScreenContainer
, and will handle it in his scope.Else there is the changes for the entire screen 👍
Before:
After: