Wallet - token metrics #18438
Merged
Wallet - token metrics #18438
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
Jenkins BuildsClick to see older builds (20)
|
smohamedjavid
requested review from
shivekkhurana,
vkjr,
briansztamfater,
J-Son89,
OmarBasem,
mmilad75 and
ulisesmac
ulisesmac
reviewed
Jan 10, 2024
Comment on lines
+185
to
+230
| (defn prettify-percentage-change | ||
| "Returns unsigned precentage" | ||
| [percentage] | ||
| (-> (if (number? percentage) percentage 0) | ||
| money/bignumber | ||
| money/absolute-value | ||
| (money/to-fixed 2))) |
There was a problem hiding this comment.
I'm not sure about this if since the logic is not related to the function's purpose.
Why do we want to keep these kinds of checks? I mean, is someone calling this function not using a number and willing to get a "$0.00" as a result? and if so, maybe we shouldn't have initially mixed non-number values with numbers.
There was a problem hiding this comment.
The idea is to prevent the method from crashing on unexpected values such as nil or strings (token market values sometimes give out such values in rare cases) and to provide a uniform result 0.00.
Comment on lines
207
to
210
| crypto-value (get-standard-crypto-format token token-units) | ||
| formatted-fiat-value (if (string/includes? crypto-value "<") | ||
| "<$0.01" | ||
| (prettify-balance currency-symbol fiat-value))] |
There was a problem hiding this comment.
Why do we depend on a string containing a <?
Seems we should compare the values directly instead of using the result of get-standard-crypto-format.
There was a problem hiding this comment.
@ulisesmac If the crypto-value includes < it means its value is less than one USD cent. In that case we display "<$0.01" for fiat.
There was a problem hiding this comment.
@OmarBasem
Yeah, but I wanted to say that we should instead compare that the value us less than 0.01, instead of checking if < is in the string.
The reason is the logic is more clear and we wouldn't rely on a formatted string that is expected to be consumed by the user. If designers decide to change < for less than it should only affect UIs
There was a problem hiding this comment.
I was checking with Desktop team yesterday about that. We need to get the formatting from status-go. We can create an issue for refactoring in Feb.
| (is (= (utils/prettify-percentage-change 0.5) "0.50")) | ||
| (is (= (utils/prettify-percentage-change 1.113454) "1.11")) | ||
| (is (= (utils/prettify-percentage-change -0.35) "0.35")) | ||
| (is (= (utils/prettify-percentage-change -0.78234) "0.78")))) |
There was a problem hiding this comment.
Shouldn't this be `"-0.78"?
There was a problem hiding this comment.
@ulisesmac - We need to display the absolute value (unsigned number) in the UI.
If the value is negative (loss), we display it in red color. If it's positive (profit), we display it in green color.
There was a problem hiding this comment.
Thanks for the explanation! :)
ulisesmac
approved these changes
Jan 11, 2024
OmarBasem
approved these changes
Jan 12, 2024
smohamedjavid
force-pushed
the
feature/wallet-token-metrics
branch
from
dd0aef1 to
5e99f87
Compare
90% of end-end tests have passedFailed tests (2)Click to expandClass TestDeepLinksOneDevice:
Expected to fail tests (3)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityOneDeviceMerged:
Passed tests (43)Click to expandClass TestActivityMultipleDevicePRTwo:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
|
pavloburykh
force-pushed
the
feature/wallet-token-metrics
branch
from
5e99f87 to
e2788bb
Compare
|
Hi @smohamedjavid thank you for PR. Here is a found issue: ISSUE 1: Incorrect calculation of price change percentage in for assetsSteps:
Actual result:The displayed
Expected result:The price change and percentage are calculated accurately |
ISSUE 2: The gray neutral state is not shown if percentage is not changedSteps:
Actual result:Some tokens are still shown with red/green marks even when the price has not changed.
Expected result:Tokens where the price is unchanged should have the gray state. |
smohamedjavid
force-pushed
the
feature/wallet-token-metrics
branch
2 times, most recently
from
2554222 to
4ddd93e
Compare
|
@VolodLytvynenko - Apologies for the late response 🙏 The rounding-off decimals create a bad UX in cases where the change percentage is very small like For the time being, we need to update the outcome of this PR to display the token prices just as the desktop does. I have updated this PR, Please check. |
29% of end-end tests have passedFailed tests (33)Click to expandClass TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestDeepLinksOneDevice:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (14)Click to expandClass TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
|
56% of end-end tests have passedFailed tests (20)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestDeepLinksOneDevice:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (27)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
|
J-Son89
reviewed
Feb 5, 2024
| {:token (:symbol token) | ||
| :token-name (:name token) | ||
| :state :default | ||
| :metrics? true |
There was a problem hiding this comment.
not for this pr but is metrics always on? 🤔 - or is there some cases where we don't want it?
There was a problem hiding this comment.
For now, yes (as we show the token market price).
In future, we will be showing the fiat change where we would hide the metrics for zero balance tokens.
J-Son89
approved these changes
Feb 5, 2024
VolodLytvynenko
force-pushed
the
feature/wallet-token-metrics
branch
from
4ddd93e to
d0e7e16
Compare
38% of end-end tests have passedFailed tests (29)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePRTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (18)Click to expandClass TestDeepLinksOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
|
81% of end-end tests have passedFailed tests (8)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityMultipleDeviceMerged:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (39)Click to expandClass TestDeepLinksOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
|
VolodLytvynenko
added
Tested - OK
and removed
Tested - Issues
test cases required
labels
|
@smohamedjavid Thank you for PR. No issues from my side. Ready to be merged |
Signed-off-by: Mohamed Javid <19339952+smohamedjavid@users.noreply.github.com>
smohamedjavid
force-pushed
the
feature/wallet-token-metrics
branch
from
d0e7e16 to
9804fe9
Compare
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.
fixes #18367
Summary
This PR adds a feature to display token prices for each token in the Wallet home and Account screens.
Platforms
Steps to test
status: ready