Feature/colored asset icons fix #1704
Merged
Conversation
This file contains 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
antonijzelinskij
changed the base branch from
develop
to
feature/portfolio-view-flows
valentunn
approved these changes
Nov 1, 2024
Comment on lines
13
to
15
| fun getAssetIcon(iconName: String?, fallback: Icon = fallbackIcon): Icon | ||
|
|
||
| fun getWhiteAssetIcon(iconName: String?, fallback: Icon = fallbackIcon): Icon |
There was a problem hiding this comment.
I gave a few suggestions for the interface here:
- The idea of having fallback icon being exposed by AssetIconProvider looks good. However I think we can make the the interface clearer if
getAssetIconwont allow for nullableiconName. Instead, we can create an extension function that allows nullability and has the same signature that is currently present in the interface directly. This way we will simplify the interface itself - Instead of creating a separate instance of
getWhiteAssetIconhow about we make a functiongetAssetIcon(iconName, iconType)and implementgetWhiteAssetIconin a form of extension? This way you wont need to icon formatting logic twice sincegetAssetIcon(iconName)can easily delegate its work togetAssetIcon(iconName, iconType)by passingiconType=selectedIconType
...e-assets/src/main/java/io/novafoundation/nova/feature_assets/di/AssetsFeatureDependencies.kt
Show resolved
Hide resolved
| @@ -18,13 +18,13 @@ class AssetsListInteractor( | |||
| private val accountRepository: AccountRepository, | |||
| private val nftRepository: NftRepository, | |||
| private val bannerVisibilityRepository: BannerVisibilityRepository, | |||
| private val assetsViewModeRepository: AssetsViewModeRepository | |||
| private val assetsViewModeService: AssetsViewModeRepository | |||
There was a problem hiding this comment.
We should change it back to assetsViewModeRepository to much the class name
Comment on lines
68
to
70
| val currentChains = chainDao.joinChainInfoFlow().map { chains -> | ||
| chains.map { mapChainLocalToChain(it, gson) } | ||
| } |
There was a problem hiding this comment.
We can revert it back to mapList
Base automatically changed from
feature/portfolio-view-flows
to
portfolio-view-base
November 1, 2024 08:35
valentunn
reviewed
Nov 1, 2024
|
|
||
| fun getAssetIcon(iconName: String?, fallback: Icon = fallbackIcon): Icon | ||
| fun getAssetIconOrFallback(iconName: String): Icon |
There was a problem hiding this comment.
Why there is orFallback in the name?
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.
#86966j47d
