Redesign Currency screen and fix broken rate sources - #130
Merged
Conversation
Contributor
|
search bar not accurate / not working.. |
Contributor
|
Contributor
Author
INR is selected already, so doesnot show up in search. can you try with some other currency like usd, etc. |
Contributor
not working WhatsApp.Video.2026-07-19.at.4.57.16.PM.mp4 |
theanmolsharma
requested changes
Jul 27, 2026
theanmolsharma
left a comment
theanmolsharma
left a comment
Collaborator
There was a problem hiding this comment.
Follow-up pass, code quality only — none of this is a bug, and none of it blocks. The changes-requested review above still holds the things I think need action.
The two I'd actually like a decision on are inline: the generated Android asset, and whether SettingsRow is meant to replace ListItem or live alongside it.
Remaining small stuff, not worth its own thread each:
styles.transparentis fighting an abstraction. It exists only to override thebackgroundColorthatSafeAreaFlatListhardcodes. That component assumes it's the screen root — nesting it inside a card is off-label. A plainFlatListhere would be more honest, since the card already handles the background.- Naming.
SettingsRow.tsxdefault-exports a component namedRow, so it shows asRowin DevTools and gets imported under a name that doesn't match its file. Same idea withroundIcononRowmapping tocircleonSettingsIconWrapper— two names for one concept. - The three stub screens are byte-identical. If they have to land, one shared
PlaceholderScreenis less to delete later. lockey pairing. Every new*_subtitlehas a matching sibling exceptsecurity_subtitle, which pairs with the pre-existingencrypt_title. Slightly surprising to grep for.
And credit where it's due — extracting SettingsRow + SettingsIconWrapper instead of inlining the layout twice is the right instinct, and the six new theme tokens are correctly mirrored across both themes (dark spreads ShroudDefaultTheme.colors, so nothing dangles). The comments below are about finishing those two ideas, not backing out of them.
The Currency.tsx redesign dropped the last references to loc._.never that find-unused-loc.js could see, since the script never scans loc/ itself where transactionTimeToReadable() still uses that key. Add loc/ to the scanned dirs to close that gap, and fix two real lint errors the unused-loc gate had been masking: a missing colors.successCheck dependency in Currency's renderItem callback, and an inline-style ternary in SettingsIconWrapper.
Drops the dead CoinDesk/cryptocompare source (COP/KES now on Coinbase) and surfaces Kraken's own error message instead of a generic one. Fixes the double-tap bug on search results (keyboardShouldPersistTaps), restores loading feedback via a new SettingsRow isLoading prop, keeps item.country in the search-match set while still ranking by short name, and moves SettingsRow's separator onto the Pressable's own border so ITEM_HEIGHT is exact. Also fixes SearchIcon sizing, listCard's safe-area bottom margin, and adds accessibilityState to SettingsRow.
Untrack android/app/src/main/assets/fiatUnits.json - it's a build artifact copied from models/fiatUnits.json by the copyFiatUnits Gradle task on every build, so hand-editing it in git is a standing trap for silent drift. Gitignore it like the sibling generated index.android.bundle. Add a comment noting Kraken's XXBTZ pair prefix only covers its Z-class quote assets (USD/EUR/GBP today) and would need a different prefix for others. Memoize SettingsRow and the icon/rightElement elements passed into it from Settings.tsx and Currency.tsx, so React.memo has stable props to compare against instead of a fresh element on every render.
theanmolsharma
approved these changes
Jul 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Currency Screen
Search Bar