-
Notifications
You must be signed in to change notification settings - Fork 86
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
feat(dapps): Redesign discover tab and dapps screen #5569
Conversation
src/dapps/DappScreen.test.tsx
Outdated
@@ -0,0 +1,718 @@ | |||
import { fireEvent, render, within } from '@testing-library/react-native' |
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 entire file is more or less copy-pasted from the TabDiscover.test.tsx
file, with only minor changes to remove now-irrelevant tests and change testID
values.
src/dapps/DappScreen.tsx
Outdated
@@ -0,0 +1,367 @@ | |||
import { NativeStackScreenProps } from '@react-navigation/native-stack' |
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 file is also more or less just copy-pasted from TabDiscover.tsx
, with only minor changes.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5569 +/- ##
==========================================
+ Coverage 86.48% 86.51% +0.02%
==========================================
Files 769 771 +2
Lines 31515 31613 +98
Branches 5453 5171 -282
==========================================
+ Hits 27255 27349 +94
- Misses 4028 4221 +193
+ Partials 232 43 -189
... and 79 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.
|
||
return ( | ||
<View testID="DiscoverDappsCard" style={styles.safeAreaContainer}> | ||
<AnimatedSectionList |
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.
Do we still need a scrollable list here?
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.
Do you mean just using a plain SectionList
instead?
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.
Even removing the need for it, since we just need max 10 items? and we don't need scroll since the parent is already scrollable.
But maybe it's easier as is, as the logic is shared with the screen?
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 helps to deal with the section headers, since it's not guaranteed that both sections will always be present. I kind of like keeping it since it's the same logic we use elsewhere for maintaining lists with distinct sections.
Thanks for the comments about the design details @jeanregisser, new video showing the styling updates here: dapps-redesign-2-2024-06-25_19.40.44.mp4 |
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.
🚀
Already approved with a small question about the need for the SectionList
in the card.
Description
For RET-1089. Redesigns the Discover tab and moves the primary dapp section back to its own screen.
Test plan
Manual and unit tested. See video below.
dapps-redesign-2024-06-24_17.15.11.mp4
Related issues
Backwards compatibility
Yes.
Network scalability
If a new NetworkId and/or Network are added in the future, the changes in this PR will: