Skip to content
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

Merged
merged 13 commits into from
Jun 26, 2024
Merged

Conversation

jophish
Copy link
Contributor

@jophish jophish commented Jun 24, 2024

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:

  • Continue to work without code changes, OR trigger a compilation error (guaranteeing we find it when a new network is added)

@@ -0,0 +1,718 @@
import { fireEvent, render, within } from '@testing-library/react-native'
Copy link
Contributor Author

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.

@@ -0,0 +1,367 @@
import { NativeStackScreenProps } from '@react-navigation/native-stack'
Copy link
Contributor Author

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.

Copy link

codecov bot commented Jun 25, 2024

Codecov Report

Attention: Patch coverage is 97.51244% with 5 lines in your changes missing coverage. Please review.

Project coverage is 86.51%. Comparing base (db84201) to head (79c487f).

Additional details and impacted files

Impacted file tree graph

@@            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     
Files Coverage Δ
src/analytics/Events.tsx 100.00% <100.00%> (ø)
src/analytics/Properties.tsx 100.00% <ø> (ø)
src/dapps/selectors.ts 100.00% <100.00%> (ø)
src/dappsExplorer/DappCard.tsx 100.00% <100.00%> (ø)
src/dappsExplorer/DiscoverDappsCard.tsx 100.00% <100.00%> (ø)
src/dappsExplorer/TabDiscover.tsx 93.10% <100.00%> (-2.77%) ⬇️
src/navigator/Screens.tsx 100.00% <100.00%> (ø)
src/dapps/DappsScreen.tsx 95.79% <95.79%> (ø)

... and 79 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update db84201...79c487f. Read the comment docs.

Copy link
Member

@jeanregisser jeanregisser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! 👍

Looks good overall, but I noticed some small UI discrepancies with the designs in the card:

design implementation
Screenshot 2024-06-25 at 11 20 45 Screenshot 2024-06-25 at 11 19 32
  • Icons should be left aligned with the section title ("MOST POPULAR")
  • It seems there's too much horizontal padding in the card

src/dapps/DappScreen.tsx Outdated Show resolved Hide resolved
src/dappsExplorer/DappCard.tsx Outdated Show resolved Hide resolved
src/dappsExplorer/DappCard.tsx Outdated Show resolved Hide resolved
src/navigator/Screens.tsx Outdated Show resolved Hide resolved
src/dappsExplorer/DiscoverDappsCard.tsx Outdated Show resolved Hide resolved

return (
<View testID="DiscoverDappsCard" style={styles.safeAreaContainer}>
<AnimatedSectionList
Copy link
Member

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?

Copy link
Contributor Author

@jophish jophish Jun 25, 2024

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?

Copy link
Member

@jeanregisser jeanregisser Jun 26, 2024

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?

Copy link
Contributor Author

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.

@jophish
Copy link
Contributor Author

jophish commented Jun 25, 2024

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

Copy link
Member

@jeanregisser jeanregisser left a 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.

src/dappsExplorer/DiscoverDappsCard.tsx Outdated Show resolved Hide resolved
@jophish jophish added this pull request to the merge queue Jun 26, 2024
Merged via the queue into main with commit d3145ee Jun 26, 2024
16 checks passed
@jophish jophish deleted the jophish/ret-1089 branch June 26, 2024 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants