-
Notifications
You must be signed in to change notification settings - Fork 80
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
chore: remove nft gallery and old token details #5016
Conversation
src/components/TokenBalance.tsx
Outdated
} | ||
|
||
return ( | ||
<View style={styles.container} testID="FiatExchangeTokenBalance"> | ||
<View style={styles.titleExchange}> | ||
<View style={styles.row}> | ||
{tokenBalances.length > 1 ? ( |
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 behavior was updated because the tests were coupled together. Now the behavior of the home token balances and the fiat exchange token balances matches.
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.
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.
The behavior is different when show_asset_details_screen was set to true which is 100%. If we want to keep the behavior of the existing FiatExchangeTokenBalance some tests will need to be updated. Due to the coupled testing it was unclear if we ever expected these two components to diverge in behavior like they 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.
I believe the FiatExchangeTokenBalance behavior has always been different from the HomeTokenBalance behavior.
FiatExchangeTokenBalance shows the ViewBalances touchable if num tokens > 1 both before and after the asset details change. HomeTokenBalance before asset details only included the ViewBalances touchable if num tokens >= 1 and after asset details, it is always included (since we also show tokens with zero balance in the assets screen).
I think we had coupled tests prior just because the case for 0 tokens overlapped. Can't we just drop the it.each
and just keep the test for FiatExchangeTokenBalance
and keep this test for HomeTokenBalance
and just remove the feature gate in the name and mocks? No new tests need to be added as part of this change.
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.
src/components/TokenBalance.test.tsx
Outdated
.mockImplementation( | ||
(featureGate) => featureGate !== StatsigFeatureGates.SHOW_ASSET_DETAILS_SCREEN | ||
) | ||
jest.mocked(getFeatureGate).mockImplementation(() => true) |
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 controlling multiple features flags. Open to other options on how to handle this.
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.
Seems like the TokenBalance.tsx only references SHOW_HIDE_HOME_BALANCES_TOGGLE
gate now that the SHOW_ASSET_DETAILS_SCREEN
gate is removed?
Could change this and all below references to this (and just update true / false appropriately)
jest.mocked(getFeatureGate).mockImplementation(() => true) | |
jest.mocked(getFeatureGate).mockReturnValue(true) |
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.
Fixed in 6fb2c1d!
1 build decreased size
Celo (test) 1.79.0 (144)
|
Item | Install Size Change |
---|---|
main.jsbundle | ⬇️ -12.3 kB |
🛸 Powered by Emerge Tools
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5016 +/- ##
==========================================
- Coverage 85.48% 85.46% -0.03%
==========================================
Files 723 719 -4
Lines 29437 29199 -238
Branches 5099 5038 -61
==========================================
- Hits 25165 24955 -210
+ Misses 4037 4014 -23
+ Partials 235 230 -5
... and 7 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
src/components/TokenBalance.test.tsx
Outdated
.mockImplementation( | ||
(featureGate) => featureGate !== StatsigFeatureGates.SHOW_ASSET_DETAILS_SCREEN | ||
) | ||
jest.mocked(getFeatureGate).mockImplementation(() => true) |
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.
Seems like the TokenBalance.tsx only references SHOW_HIDE_HOME_BALANCES_TOGGLE
gate now that the SHOW_ASSET_DETAILS_SCREEN
gate is removed?
Could change this and all below references to this (and just update true / false appropriately)
jest.mocked(getFeatureGate).mockImplementation(() => true) | |
jest.mocked(getFeatureGate).mockReturnValue(true) |
src/components/TokenBalance.tsx
Outdated
} | ||
|
||
return ( | ||
<View style={styles.container} testID="FiatExchangeTokenBalance"> | ||
<View style={styles.titleExchange}> | ||
<View style={styles.row}> | ||
{tokenBalances.length > 1 ? ( |
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.
src/components/TokenBalance.test.tsx
Outdated
) | ||
|
||
it.each([HomeTokenBalance, FiatExchangeTokenBalance])( | ||
it.each([HomeTokenBalance])( |
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.
Since this applies to only home token balance, can we move this to its own
describe section? This would also allow us to change the it.each
to a
describe.each
so we don't end up adding tests in this section that
only applies to one of them?
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.
Fixed in 047325d!
describe('FiatExchangeTokenBalance', () => { | ||
beforeEach(() => { | ||
jest.clearAllMocks() | ||
jest.mocked(getFeatureGate).mockReturnValue(true) |
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.
is this required? this component doesn't use a feature gate right?
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 started failing when I removed the feature gate.
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.
Ah, looks like this is needed for positions to be included. I think we also need this for the describe('FiatExchangeTokenBalance and HomeTokenBalance') section too. I think they currently pass from a stray mock from another describe. We should add that back, because I think running them standalone would fail
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.
A once getFeatureGate mock is added
describe('FiatExchangeTokenBalance', () => { | ||
beforeEach(() => { | ||
jest.clearAllMocks() | ||
jest.mocked(getFeatureGate).mockReturnValue(true) |
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.
Ah, looks like this is needed for positions to be included. I think we also need this for the describe('FiatExchangeTokenBalance and HomeTokenBalance') section too. I think they currently pass from a stray mock from another describe. We should add that back, because I think running them standalone would fail
src/components/TokenBalance.test.tsx
Outdated
@@ -737,6 +419,262 @@ describe('FiatExchangeTokenBalance and HomeTokenBalance', () => { | |||
}) | |||
}) | |||
|
|||
describe('FiatExchangeTokenBalance and HomeTokenBalance', () => { |
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.
nit: can we just do a describe.each and drop the it.each everywhere else?
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.
Fixed in 0459fa9!
### Description Cleanup of old Nft Gallery feature now handled in Assets and of the old token balances screen also now handled in assets. Additionally the following feature flags which have been rolled out to 100% were removed: `show_in_app_nft_gallery`, `show_in_app_nft_viewer`, `view_nft_home_assets` and `show_asset_details_screen`. ### Test plan - Tested locally on iOS - E2E tests passing ### Related issues - Fixes ACT-1074 ### Backwards compatibility Yes ### Network scalability Yes
Description
Cleanup of old Nft Gallery feature now handled in Assets and of the old token balances screen also now handled in assets. Additionally the following feature flags which have been rolled out to 100% were removed:
show_in_app_nft_gallery
,show_in_app_nft_viewer
,view_nft_home_assets
andshow_asset_details_screen
.Test plan
Related issues
Backwards compatibility
Yes
Network scalability
Yes