-
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 old CeloGoldHistoryChart #5307
Conversation
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.
Nice cleanup! Needs resolving conflicts
@@ -1,5 +1,6 @@ | |||
import { parsePhoneNumber } from '@celo/phone-utils' | |||
import { ADDRESS_LENGTH } from 'src/exchange/reducer' | |||
|
|||
const ADDRESS_LENGTH = 42 | |||
|
|||
export const isAddressFormat = (content: string): boolean => { |
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.
Not directly related to your PR, but guess this is something we could replace with a viem helper, which I believe we use in the qr scanner. Maybe leave a note / make a tech debt issue?
@@ -1744,4 +1753,7 @@ export const migrations = { | |||
pointsConfigStatus: 'idle', | |||
}, | |||
}), | |||
208: (state: any) => ({ | |||
...(_.omit(state, 'exchange') as any), |
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 the as any
required?
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.
yeah without it I get errors when running yarn build
, the as any
is used in a couple other places where an omit like this is done
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5307 +/- ##
==========================================
+ Coverage 85.92% 86.04% +0.12%
==========================================
Files 738 735 -3
Lines 30173 29948 -225
Branches 5168 5123 -45
==========================================
- Hits 25925 25770 -155
+ Misses 4016 3951 -65
+ Partials 232 227 -5
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
e2e/src/usecases/Assets.js
Outdated
await scrollIntoView(`Learn more about ${name}`, 'TokenDetailsScrollView') | ||
await waitForElementByIdAndTap('TokenDetails/LearnMore') |
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.
optional nit: seems odd to use text for scrolling and id for tapping. would be nice if this was consistent
const tokensBySymbol = useSelector(tokensBySymbolSelector) | ||
|
||
const onTap = useCallback(() => { | ||
ValoraAnalytics.track(CeloExchangeEvents.celo_chart_tapped) |
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.
can we remove this analytics event?
1 build decreased size
Celo (test) 1.83.0 (148)
|
Item | Install Size Change |
---|---|
main.jsbundle | ⬇️ -16.4 kB |
🛸 Powered by Emerge Tools
@@ -3286,6 +3286,14 @@ export const v210Schema = { | |||
}, | |||
} | |||
|
|||
export const v211Schema = { |
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.
@finnian0826 Just noticed this when attempting to add a new schema in a separate PR, but I think there's a missing schema here.
...v210Schema,
..._.omit(v210Schema, 'exchange'),
_persist: {
...v210Schema._persist,
version: 211,
},
}
### Description - Usage of CeloGoldHistoryChart removed - exchangeHistorySelector usage replaced with token.historicalPricesUsd.lastDay - sage/reducer/action cleaned up - exchange folder remaned to celoNews ### Test plan builds, root state schema updated ### Related issues - Fixes #ACT-1039 ### Backwards compatibility Yes ### Network scalability If a new NetworkId and/or Network are added in the future, the changes in this PR will: - N/A
Description
Test plan
builds, root state schema updated
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: