search

status-im / status-mobile

a free (libre) open source, mobile OS for Ethereum
https://status.app
Mozilla Public License 2.0
3.92k stars 984 forks source link

Wallet design review fixes #21542

Closed clauxx closed 1 week ago

clauxx commented 3 weeks ago

fixes #21223 fixes #21224 fixes #21221

Summary

Some of the design issues have been already fixed it seems, as I couldn't reproduce them, so this is what was fixed here:

  1. hide the "dApp connections" button from the header, unless we're on the account screen
  2. hide the "token metrics" (price change indicator) if token has no balance

Not sure about using EUR as the default currency, as that would change it on desktop as well. @shivekkhurana is there a decision on using EUR as the default currency?

Screenshots





status: ready

status-im-auto commented 3 weeks ago

Jenkins Builds

Click to see older builds (12) | :grey_question: | Commit | :hash: | Finished (UTC) | Duration | Platform | Result | |-|-|-|-|-|-|-| | :heavy_check_mark: | 259204fc | [#1](https://ci.status.im/job/status-mobile/job/prs/job/tests/job/PR-21542/1/) | 2024-10-31 12:16:17 | ~5 min | `tests` | [:page_facing_up:`log`](https://status-im-mobile-prs.ams3.cdn.digitaloceanspaces.com/StatusIm-Mobile-241031-121112-259204-pr21542-tests.log) | | :heavy_check_mark: | 259204fc | [#1](https://ci.status.im/job/status-mobile/job/prs/job/android-e2e/job/PR-21542/1/) | 2024-10-31 12:19:25 | ~8 min | `android-e2e` | [:robot:`apk`](https://status-im-mobile-prs.ams3.cdn.digitaloceanspaces.com/StatusIm-Mobile-241031-121112-259204-pr21542-x86_64.apk) [:calling:](https://chart.apis.google.com/chart?cht=qr&chs=400x400&chld=L%7C%0A1&chl=https%3A%2F%2Fstatus-im-mobile-prs.ams3.cdn.digitaloceanspaces.com%2FStatusIm-Mobile-241031-121112-259204-pr21542-x86_64.apk)| | :heavy_check_mark: | 259204fc | [#1](https://ci.status.im/job/status-mobile/job/prs/job/android/job/PR-21542/1/) | 2024-10-31 12:19:57 | ~8 min | `android` | [:robot:`apk`](https://status-im-mobile-prs.ams3.cdn.digitaloceanspaces.com/StatusIm-Mobile-241031-121112-259204-pr21542-arm64-v8a.apk) [:calling:](https://chart.apis.google.com/chart?cht=qr&chs=400x400&chld=L%7C%0A1&chl=https%3A%2F%2Fstatus-im-mobile-prs.ams3.cdn.digitaloceanspaces.com%2FStatusIm-Mobile-241031-121112-259204-pr21542-arm64-v8a.apk)| | :heavy_check_mark: | 259204fc | [#1](https://ci.status.im/job/status-mobile/job/prs/job/ios/job/PR-21542/1/) | 2024-10-31 12:20:35 | ~9 min | `ios` | [:iphone:`ipa`](https://i.diawi.com/7PikHX) [:calling:](https://chart.apis.google.com/chart?cht=qr&chs=400x400&chld=L%7C%0A1&chl=https%3A%2F%2Fi.diawi.com%2F7PikHX)| | | | | | | | | | :heavy_check_mark: | a467c69c | [#3](https://ci.status.im/job/status-mobile/job/prs/job/tests/job/PR-21542/3/) | 2024-11-04 12:09:17 | ~4 min | `tests` | [:page_facing_up:`log`](https://status-im-mobile-prs.ams3.cdn.digitaloceanspaces.com/StatusIm-Mobile-241104-120450-a467c6-pr21542-tests.log) | | :heavy_check_mark: | a467c69c | [#3](https://ci.status.im/job/status-mobile/job/prs/job/android-e2e/job/PR-21542/3/) | 2024-11-04 12:12:25 | ~7 min | `android-e2e` | [:robot:`apk`](https://status-im-mobile-prs.ams3.cdn.digitaloceanspaces.com/StatusIm-Mobile-241104-120445-a467c6-pr21542-x86_64.apk) [:calling:](https://chart.apis.google.com/chart?cht=qr&chs=400x400&chld=L%7C%0A1&chl=https%3A%2F%2Fstatus-im-mobile-prs.ams3.cdn.digitaloceanspaces.com%2FStatusIm-Mobile-241104-120445-a467c6-pr21542-x86_64.apk)| | :heavy_check_mark: | a467c69c | [#3](https://ci.status.im/job/status-mobile/job/prs/job/android/job/PR-21542/3/) | 2024-11-04 12:12:39 | ~7 min | `android` | [:robot:`apk`](https://status-im-mobile-prs.ams3.cdn.digitaloceanspaces.com/StatusIm-Mobile-241104-120445-a467c6-pr21542-arm64-v8a.apk) [:calling:](https://chart.apis.google.com/chart?cht=qr&chs=400x400&chld=L%7C%0A1&chl=https%3A%2F%2Fstatus-im-mobile-prs.ams3.cdn.digitaloceanspaces.com%2FStatusIm-Mobile-241104-120445-a467c6-pr21542-arm64-v8a.apk)| | :heavy_check_mark: | a467c69c | [#3](https://ci.status.im/job/status-mobile/job/prs/job/ios/job/PR-21542/3/) | 2024-11-04 12:14:53 | ~10 min | `ios` | [:iphone:`ipa`](https://i.diawi.com/RRBzvE) [:calling:](https://chart.apis.google.com/chart?cht=qr&chs=400x400&chld=L%7C%0A1&chl=https%3A%2F%2Fi.diawi.com%2FRRBzvE)| | | | | | | | | | :heavy_check_mark: | 08fd79d7 | [#5](https://ci.status.im/job/status-mobile/job/prs/job/tests/job/PR-21542/5/) | 2024-11-05 14:26:29 | ~4 min | `tests` | [:page_facing_up:`log`](https://status-im-mobile-prs.ams3.cdn.digitaloceanspaces.com/StatusIm-Mobile-241105-142129-08fd79-pr21542-tests.log) | | :heavy_check_mark: | 08fd79d7 | [#5](https://ci.status.im/job/status-mobile/job/prs/job/ios/job/PR-21542/5/) | 2024-11-05 14:31:06 | ~9 min | `ios` | [:iphone:`ipa`](https://i.diawi.com/RLQPbM) [:calling:](https://chart.apis.google.com/chart?cht=qr&chs=400x400&chld=L%7C%0A1&chl=https%3A%2F%2Fi.diawi.com%2FRLQPbM)| | :heavy_check_mark: | 08fd79d7 | [#5](https://ci.status.im/job/status-mobile/job/prs/job/android-e2e/job/PR-21542/5/) | 2024-11-05 14:31:12 | ~9 min | `android-e2e` | [:robot:`apk`](https://status-im-mobile-prs.ams3.cdn.digitaloceanspaces.com/StatusIm-Mobile-241105-142129-08fd79-pr21542-x86_64.apk) [:calling:](https://chart.apis.google.com/chart?cht=qr&chs=400x400&chld=L%7C%0A1&chl=https%3A%2F%2Fstatus-im-mobile-prs.ams3.cdn.digitaloceanspaces.com%2FStatusIm-Mobile-241105-142129-08fd79-pr21542-x86_64.apk)| | :heavy_check_mark: | 08fd79d7 | [#5](https://ci.status.im/job/status-mobile/job/prs/job/android/job/PR-21542/5/) | 2024-11-05 14:31:42 | ~10 min | `android` | [:robot:`apk`](https://status-im-mobile-prs.ams3.cdn.digitaloceanspaces.com/StatusIm-Mobile-241105-142129-08fd79-pr21542-arm64-v8a.apk) [:calling:](https://chart.apis.google.com/chart?cht=qr&chs=400x400&chld=L%7C%0A1&chl=https%3A%2F%2Fstatus-im-mobile-prs.ams3.cdn.digitaloceanspaces.com%2FStatusIm-Mobile-241105-142129-08fd79-pr21542-arm64-v8a.apk)|
:grey_question: Commit :hash: Finished (UTC) Duration Platform Result
:x: 8b60b1d6 #6 2024-11-08 10:57:50 ~2 min ios :page_facing_up:log
:heavy_check_mark: 8b60b1d6 #6 2024-11-08 11:00:02 ~4 min tests :page_facing_up:log
:heavy_check_mark: 8b60b1d6 #6 2024-11-08 11:01:48 ~6 min android-e2e :robot:apk :calling:
:heavy_check_mark: 8b60b1d6 #6 2024-11-08 11:03:49 ~8 min android :robot:apk :calling:
:heavy_check_mark: e8caad05 #7 2024-11-19 10:03:08 ~8 min tests :page_facing_up:log
:heavy_check_mark: e8caad05 #7 2024-11-19 10:04:36 ~9 min android-e2e :robot:apk :calling:
:heavy_check_mark: e8caad05 #7 2024-11-19 10:04:51 ~10 min android :robot:apk :calling:
:heavy_check_mark: e8caad05 #7 2024-11-19 10:04:56 ~10 min ios :iphone:ipa :calling:
shivekkhurana commented 3 weeks ago

@shivekkhurana is there a decision on using EUR as the default currency? I think at the moment, we are forced to. Because we don't have a currency switcher on mobile.

@smohamedjavid @xAlisher Do you know if EURO is the default by choice or by force ?

clauxx commented 3 weeks ago

@shivekkhurana is there a decision on using EUR as the default currency? I think at the moment, we are forced to. Because we don't have a currency switcher on mobile.

@smohamedjavid @xAlisher Do you know if EURO is the default by choice or by force ?

@shivekkhurana we do have a currency switcher on mobile though :thinking:

smohamedjavid commented 3 weeks ago

@shivekkhurana is there a decision on using EUR as the default currency? I think at the moment, we are forced to. Because we don't have a currency switcher on mobile.

@smohamedjavid @xAlisher Do you know if EURO is the default by choice or by force ?

I believe we have been using USD as the default currency for both clients. If I recall correctly, there was a discussion very long ago to make EUR the default, but it's not done yet (not sure it's captured somewhere).

Additionally, We also rely on USD, especially for rounding off decimals in the token balance https://github.com/status-im/status-mobile/blob/9862abb7ebaac387652b9f3ff263b96198a3679f/src/status_im/contexts/wallet/common/utils.cljs#L113-L127

smohamedjavid commented 3 weeks ago

@shivekkhurana we do have a currency switcher on mobile though 🤔

@clauxx - Yes, we do have. It can be done through Profile > Language and currency > Currency. If the user changes currency on any of the clients, the synced devices will be updated too.

status-im-auto commented 3 weeks ago

75% of end-end tests have passed

Total executed tests: 8
Failed tests: 2
Expected to fail tests: 0
Passed tests: 6
IDs of failed tests: 703133,702843

Failed tests (2)

Click to expand
  • Rerun failed tests

    • Class TestCommunityMultipleDeviceMerged:

    1. test_community_message_edit, id: 702843

    Device 2: Find `Text` by `xpath`: `//android.view.ViewGroup[@content-desc='chat-item']//android.widget.TextView[contains(@text,'https://status.app/c/')]`
    Device 2: Wait for element `Button` for max 120s and click when it is available

    ``` Test setup failed: critical/chats/test_public_chat_browsing.py:350: in prepare_devices self.community_2.join_community() ../views/chat_view.py:420: in join_community self.join_button.wait_and_click(120) ../views/base_element.py:100: in wait_and_click self.wait_for_visibility_of_element(sec) ../views/base_element.py:147: in wait_for_visibility_of_element raise TimeoutException( Device 2: Button by accessibility id:`show-request-to-join-screen-button` is not found on the screen after wait_for_visibility_of_element ```

    Device sessions

    Class TestCommunityOneDeviceMerged:

    1. test_restore_multiaccount_with_waku_backup_remove_switch, id: 703133

    # STEP: Check that removed user is not shown in the list anymore
    Device 1: Wait for element `Button` for max 30s and click when it is available

    ``` critical/chats/test_public_chat_browsing.py:240: in test_restore_multiaccount_with_waku_backup_remove_switch self.sign_in.show_profiles_button.wait_and_click() ../views/base_element.py:100: in wait_and_click self.wait_for_visibility_of_element(sec) ../views/base_element.py:147: in wait_for_visibility_of_element raise TimeoutException( Device 1: Button by accessibility id:`show-profiles` is not found on the screen after wait_for_visibility_of_element ```

    Device sessions

    Passed tests (6)

    Click to expand

    Class TestOneToOneChatMultipleSharedDevicesNewUi:

    1. test_1_1_chat_non_latin_messages_stack_update_profile_photo, id: 702745
    Device sessions

    Class TestWalletOneDevice:

    1. test_wallet_add_remove_regular_account, id: 727231
    2. test_wallet_balance_mainnet, id: 740490

    Class TestCommunityOneDeviceMerged:

    1. test_community_copy_and_paste_message_in_chat_input, id: 702742
    Device sessions

    Class TestWalletMultipleDevice:

    1. test_wallet_send_asset_from_drawer, id: 727230
    2. test_wallet_send_eth, id: 727229
    pavloburykh commented 3 weeks ago

    @clauxx thanks for the fixes.

    Speaking of https://github.com/status-im/status-mobile/issues/21224 I see that 2 reported points of the issue are still valid:

    Currency [1] should be set to € by default Icon in the button [2] shouldn't be there

    photo_2024-11-04 16 51 45

    I see you have already raised a question about default euros, so we need to decide what to do with that. Do we need to fix it? if yes, then where do we want to fix it: in scope of this PR or log a separate issue?

    pavloburykh commented 3 weeks ago

    Similar situation with https://github.com/status-im/status-mobile/issues/21221. Two points are still valid:

    Currency [1] should be set to € by default Bridge icon [2] still needs to be updated

    clauxx commented 3 weeks ago

    Similar situation with #21221. Two points are still valid:

    Currency [1] should be set to € by default Bridge icon [2] still needs to be updated

    @pavloburykh thanks for having a look!

    Regarding the currency, I think it's best to have a separate issue for it, as it doesn't seem like there's a clear decision on it.

    The bridge icon was fixed here, should be here now as well.

    Removed the icon in the button :+1:

    pavloburykh commented 3 weeks ago

    Thanks for the PR @clauxx. LGTM!

    Regarding the currency, I think it's best to have a separate issue for it, as it doesn't seem like there's a clear decision on it.

    I have logged a separate issue regarding default currency https://github.com/status-im/status-mobile/issues/21582.

    @Francesca-G could you please review current PR and tell if it is okay from design perspective. Following issues are fixed: https://github.com/status-im/status-mobile/issues/21223 https://github.com/status-im/status-mobile/issues/21224 https://github.com/status-im/status-mobile/issues/21221

    Mind that default EUR currency has moved to separate issue as we do not have clear decision about implementation yet.

    @clauxx once PR is approved by @Francesca-G it will be ready for merge.

    pavloburykh commented 2 weeks ago

    Looks good to me 👍

    Thank you @Francesca-G.

    @clauxx PR is ready for merge now.