-
Notifications
You must be signed in to change notification settings - Fork 94
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(earn-cards): handle large font sizes #6308
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6308 +/- ##
==========================================
- Coverage 89.03% 89.02% -0.02%
==========================================
Files 735 735
Lines 31327 31328 +1
Branches 5812 5812
==========================================
- Hits 27892 27889 -3
- Misses 3242 3394 +152
+ Partials 193 45 -148
... and 67 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
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.
flexShrink: { | ||
flexShrink: 1, | ||
flexLabel: { | ||
flex: 1.2, |
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.
Why the non-standard flex value here? Is this to make up for the lack of the paddingRight
that we used to have?
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.
This is just for spacing, as having the columns of flex 1 and 1 values appear a bit cramped and gives a bit too much room to the labels; it is the same as flex: 6 to 5, with an 11 column 11-column layout.
}: { | ||
label: string | ||
onPress?: () => void | ||
labelStyle?: StyleProp<TextStyle> | ||
iconSize?: number | ||
numberOfLines?: number | undefined |
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.
I wonder if it's better to have 1
or undefined
as the default
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.
When I tried to set the default to 1 I ran into an issue where setting it to undefined wasn't overriding the default value. I can try setting the default to undefined and see how that works.
# Conflicts: # src/earn/EarnEnterAmount.tsx
Description
Large fonts and non-English languages resulted in off-screen text and uneven columns for the earn cards used on the
src/earn/poolInfoScreen/EarnPoolInfoScreen.tsx
.Test plan
Android Large Text
Screen.Recording.2024-12-05.at.12.06.33.AM.mov
Screen.Recording.2024-12-05.at.11.30.42.AM.mov
iOS Large Text
Screen.Recording.2024-12-05.at.12.01.08.AM.mov
Screen.Recording.2024-12-05.at.11.32.06.AM.mov
Default Text (Spanish)
Related issues
N/A
Backwards compatibility
Yes
Network scalability
N/A