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

refactor: remove drawer navigation #5318

Merged
merged 11 commits into from
Apr 26, 2024
Merged

refactor: remove drawer navigation #5318

merged 11 commits into from
Apr 26, 2024

Conversation

satish-ravi
Copy link
Contributor

@satish-ravi satish-ravi commented Apr 24, 2024

Description

Cleanup drawer navigation. Includes removing:

  • DrawerNavigator and other drawer components and icons
  • All drawer screens and its associated components including (not exclusive)
    • Removing celo page
    • Removing drawer variants of screens used both in drawer and tabs (Home, Assets/Wallet, Dapps/Discover, Settings, Support, WalletSecurityPrimer, Invite)
    • Removing drawer only home screen components like beta tag, notification spotlight, dapps carousel, home token balance, etc
    • Hiding tx feed balances (now exclusively for wallet tab)
  • All other unused items flagged by knip

Will make a follow up to clean more todos including:

  • dropping the statsig gate override launch arg from e2e tests
  • dropping settings screen param in the backup quiz flow
  • renaming tab screen components
  • other misc todos like EmbeddedNavBar enum, etc

Test plan

CI, manually going through flows

Related issues

Backwards compatibility

Yes

Network scalability

N/A

Copy link

emerge-tools bot commented Apr 24, 2024

1 build decreased size

Name Version Download Change Install Change Approval
Celo (test)
org.celo.mobile.test
1.83.0 (148) 27.7 MB ⬇️ 43.9 kB (-0.16%) 65.9 MB ⬇️ 131.4 kB (-0.2%) N/A

Celo (test) 1.83.0 (148)
org.celo.mobile.test

⚖️ Compare build
📦 Install build
⏱️ Analyze build performance

Total install size change: ⬇️ 131.4 kB (-0.2%)
Total download size change: ⬇️ 43.9 kB (-0.16%)

Largest size changes

Item Install Size Change
main.jsbundle ⬇️ -106.5 kB
🗑 drawer ⬇️ -12.3 kB
Other ⬇️ -12.6 kB
View Treemap

Image of diff


🛸 Powered by Emerge Tools

Copy link

codecov bot commented Apr 24, 2024

Codecov Report

Attention: Patch coverage is 93.54839% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 85.86%. Comparing base (ab86920) to head (9e5766c).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5318      +/-   ##
==========================================
- Coverage   86.08%   85.86%   -0.22%     
==========================================
  Files         748      737      -11     
  Lines       30729    30162     -567     
  Branches     5323     5171     -152     
==========================================
- Hits        26452    25900     -552     
+ Misses       4044     4029      -15     
  Partials      233      233              
Files Coverage Δ
src/account/Education.tsx 78.16% <ø> (+1.23%) ⬆️
src/account/Settings.tsx 91.51% <ø> (-0.19%) ⬇️
src/account/Support.tsx 100.00% <100.00%> (ø)
src/app/selectors.ts 93.87% <100.00%> (-0.67%) ⬇️
src/backup/BackupIntroduction.tsx 90.24% <100.00%> (+0.44%) ⬆️
src/components/AccountNumber.tsx 87.50% <ø> (+2.20%) ⬆️
src/components/TokenBalance.tsx 98.41% <100.00%> (+0.38%) ⬆️
src/consumerIncentives/analyticsEventsTracker.ts 94.44% <ø> (-0.30%) ⬇️
src/dapps/selectors.ts 100.00% <100.00%> (ø)
src/dapps/types.ts 100.00% <ø> (ø)
... and 18 more

... and 7 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 ab86920...9e5766c. Read the comment docs.

@satish-ravi satish-ravi marked this pull request as ready for review April 24, 2024 22:48
)
expect(tree).toMatchSnapshot()
})
describe.each([{ settingsScreen: Screens.Settings }, {}])(
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this kept as a describe.each so that the settings screen is passed to routeParams?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually this describe.each (added here: https://github.com/valora-inc/wallet/pull/5121/files#diff-6215a8f5c86692eac81da63fc0f0a41ace9c791f2ad68e8d17687a175d817fedR14) isn't really doing anything because routeParams is never passed. Just runs the same test multiple times. I am cleaning this up in a follow up PR (#5336) to add better tests

@@ -259,42 +254,14 @@ describe('AssetList', () => {
})

it.each([
{
name: 'tokens tab on wallet tab when gate is off',
Copy link
Contributor

Choose a reason for hiding this comment

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

is this test removed, I don't see it below but it looks like it is for tab navigator?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch, was confused because there are different gates involved, will add it back

@@ -233,7 +217,7 @@ export function AssetsTokenBalance({
<TokenBalance
style={styles.totalBalance}
singleTokenViewEnabled={false}
showBalanceToggle={isWalletTab}
showBalanceToggle={true}
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this prop can now be removed from TokenBalance

Copy link
Contributor Author

@satish-ravi satish-ravi Apr 25, 2024

Choose a reason for hiding this comment

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

FiatExchangeTokenBalance also uses this but that doesn't need it. I think we should just make them different components (that shares a total balance selector), which can be in a follow up

@satish-ravi satish-ravi added this pull request to the merge queue Apr 25, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 26, 2024
@satish-ravi satish-ravi added this pull request to the merge queue Apr 26, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 26, 2024
@satish-ravi satish-ravi added this pull request to the merge queue Apr 26, 2024
Merged via the queue into main with commit 0249fe1 Apr 26, 2024
16 checks passed
@satish-ravi satish-ravi deleted the satish/act-1133 branch April 26, 2024 03:02
github-merge-queue bot pushed a commit that referenced this pull request Apr 26, 2024
### Description

Follow up from #5318. Remove override that is no longer required

### Test plan

CI

### Related issues

- Part of ACT-1133

### Backwards compatibility

Yes

### Network scalability

N/A
shottah pushed a commit to zed-io/kolektivo that referenced this pull request May 15, 2024
### Description

Cleanup drawer navigation. Includes removing:
- DrawerNavigator and other drawer components and icons
- All drawer screens and its associated components including (not
exclusive)
  - Removing celo page
- Removing drawer variants of screens used both in drawer and tabs
(Home, Assets/Wallet, Dapps/Discover, Settings, Support,
WalletSecurityPrimer, Invite)
- Removing drawer only home screen components like beta tag,
notification spotlight, dapps carousel, home token balance, etc
  - Hiding tx feed balances (now exclusively for wallet tab)
- All other unused items flagged by knip

 Will make a follow up to clean more todos including:
- dropping the statsig gate override launch arg from e2e tests
- dropping settings screen param in the backup quiz flow
- renaming tab screen components 
- other misc todos like EmbeddedNavBar enum, etc

### Test plan

CI, manually going through flows

### Related issues

- Fixes ACT-1133

### Backwards compatibility

Yes

### Network scalability

N/A
shottah pushed a commit to zed-io/kolektivo that referenced this pull request May 15, 2024
### Description

Follow up from valora-inc#5318. Remove override that is no longer required

### Test plan

CI

### Related issues

- Part of ACT-1133

### Backwards compatibility

Yes

### Network scalability

N/A
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.

3 participants