[Wallet] Redesign - Add header components #3803
Conversation
i1skn
force-pushed
the
i1skn/redesign-top-navigation
branch
3 times, most recently
from
0e98cd9
to
e13fd1b
Compare
Codecov Report
@@ Coverage Diff @@
## master #3803 +/- ##
=======================================
Coverage 75.50% 75.51%
=======================================
Files 608 608
Lines 15655 15660 +5
Branches 1762 1695 -67
=======================================
+ Hits 11821 11825 +4
- Misses 3469 3473 +4
+ Partials 365 362 -3
Continue to review full report at Codecov.
|
|
|
||
| const styles = StyleSheet.create({ | ||
| container: { | ||
| paddingLeft: variables.contentPadding + 7, |
There was a problem hiding this comment.
im not sure about using these variables in new design. since everything is multiples of 8. the +7 worries me
There was a problem hiding this comment.
+1, setting up new variables may be nice.
But also, I find variables for things like padding to be unhelpful. How often will we say 'reduce padding across the app by 2'. Even if we did, unlikely that most things would line up right after the var change
There was a problem hiding this comment.
I've moved this logic to mobile package. Regarding the padding itself I've changed it to +6 (correlates with the design) to the standard padding we have.
| @@ -5,7 +5,7 @@ import TopBarButton from 'src/navigator/TopBarButton.v2' | |||
|
|
|||
| describe('TopBarButton', () => { | |||
| it('displays children', () => { | |||
| const { queryByText } = render(<TopBarButton>label</TopBarButton>) | |||
| const { queryByText } = render(<TopBarButton title={'label'} />) | |||
There was a problem hiding this comment.
hmm I think we should avoid changing interfaces
There was a problem hiding this comment.
I think it should be case-by-case. A title prop does make more sense to me than a child component
|
|
||
| const styles = StyleSheet.create({ | ||
| container: { | ||
| paddingLeft: variables.contentPadding + 7, |
There was a problem hiding this comment.
+1, setting up new variables may be nice.
But also, I find variables for things like padding to be unhelpful. How often will we say 'reduce padding across the app by 2'. Even if we did, unlikely that most things would line up right after the var change
| @@ -5,7 +5,7 @@ import TopBarButton from 'src/navigator/TopBarButton.v2' | |||
|
|
|||
| describe('TopBarButton', () => { | |||
| it('displays children', () => { | |||
| const { queryByText } = render(<TopBarButton>label</TopBarButton>) | |||
| const { queryByText } = render(<TopBarButton title={'label'} />) | |||
There was a problem hiding this comment.
I think it should be case-by-case. A title prop does make more sense to me than a child component
| available | ||
| </Trans> | ||
| ) : ( | ||
| // TODO: a null balance doesn't necessarily mean it's loading |
There was a problem hiding this comment.
I find the possibility of null balance creates ugly conditionals all across the app and I wonder if we'd be better off just defaulting to 0. We'd lose a signal that says 'I don't know the balance' but it would simplify many places
There was a problem hiding this comment.
I think, this is a bit out of the scope of this PR. But I agree, we should come up with a convenient approach how we represent unknown balance.
|
@aaronmgdr I've reworked |
i1skn
force-pushed
the
i1skn/redesign-top-navigation
branch
from
cdd45cc
to
a75d6d2
Compare
i1skn
force-pushed
the
i1skn/redesign-top-navigation
branch
from
07a2c81
to
884dd38
Compare
Description
TopBarIconButtonandTopBarTextButton.HeaderTitleWithSubtitleTested
Android emulator
Backwards compatibility
Yes
Screenshots
1
2
3
4
5 (when top bar button is pressed)