chore: add outline for transaction-progress page #18217
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
J-Son89
commented
Dec 17, 2023
| @@ -15,9 +15,8 @@ | |||
| {:style style/button-container | |||
| :accessibility-label accessibility-label} | |||
| [wallet-button/view | |||
| {:icon icon | |||
| :on-press on-press | |||
| :on-long-press #(js/alert "long pressed")}] | |||
There was a problem hiding this comment.
unneeded on long press. In general we should try treat quo components as a final product and not add generic (redundant) info actions like this to these components
J-Son89
commented
Dec 17, 2023
| @@ -38,7 +38,8 @@ | |||
| (let [{:keys [name color balance watch-only?]} (rf/sub [:wallet/current-viewing-account]) | |||
| currency-symbol (rf/sub [:profile/currency-symbol])] | |||
| [rn/view {:style {:flex 1}} | |||
| [account-switcher/view {:on-press #(rf/dispatch [:wallet/close-account-page])}] | |||
| [rn/pressable {:on-press #(rf/dispatch [:navigate-to :wallet-transaction-progress])} | |||
There was a problem hiding this comment.
temporary navigation on account page above account overview. I prefer to remove this from pr but will leave while it is being reviewed if any devs want to check out the page quickly.
There was a problem hiding this comment.
Please add a note in the PR on where we need to tap (should tap on the empty area of the navigation header of the account screen). I was searching for a button or something on the page. 😄
There was a problem hiding this comment.
Sorry yeah it's pretty unclear 😆
Jenkins BuildsClick to see older builds (33)
|
J-Son89
force-pushed
the
jc/first-pass-transaction-page
branch
from
afcd9b3 to
b119898
Compare
J-Son89
requested review from
OmarBasem,
smohamedjavid,
ulisesmac,
vkjr,
mmilad75 and
shivekkhurana
J-Son89
force-pushed
the
jc/first-pass-transaction-page
branch
2 times, most recently
from
ebfb200 to
bf6f14f
Compare
J-Son89
changed the title
OmarBasem
reviewed
Dec 18, 2023
Comment on lines
55
to
56
| [{:keys [header footer customization-color gradient-cover?] :or {customization-color :blue}} & | ||
| children] |
There was a problem hiding this comment.
Maybe better not to have a default blue customization-color prop here? If someone used this component somewhere the customization color should be passed, and it wasn't, the component would still render fine. If there is no default it would should a missing prop
There was a problem hiding this comment.
the gradient cover was crashing without it, but I will move the safety check to that component instead 👍
translations/en.json
Outdated
| @@ -2435,5 +2435,8 @@ | |||
| "share-opensea-link": "Share OpenSea link", | |||
| "save-image-to-photos": "Save image to Photos", | |||
| "copy-all-details": "Copy all details", | |||
| "share-details": "Share details" | |||
| "share-details": "Share details", | |||
| "sending...": "Sending...", | |||
There was a problem hiding this comment.
Generally, we don't use ellipsis in the keys. Should we rename it to sending-with-ellipsis or something?
There was a problem hiding this comment.
sure, will adjust 👍
| (case status | ||
| :sending (i18n/label :t/sending...) | ||
| :confirmed (i18n/label :t/transaction-confirmed) | ||
| :finalised (i18n/label :t/transacation-finalised))) |
There was a problem hiding this comment.
The case doesn't have a default value. Otherwise, it will crash if it receives other values.
| @@ -38,7 +38,8 @@ | |||
| (let [{:keys [name color balance watch-only?]} (rf/sub [:wallet/current-viewing-account]) | |||
| currency-symbol (rf/sub [:profile/currency-symbol])] | |||
| [rn/view {:style {:flex 1}} | |||
| [account-switcher/view {:on-press #(rf/dispatch [:wallet/close-account-page])}] | |||
| [rn/pressable {:on-press #(rf/dispatch [:navigate-to :wallet-transaction-progress])} | |||
There was a problem hiding this comment.
Please add a note in the PR on where we need to tap (should tap on the empty area of the navigation header of the account screen). I was searching for a button or something on the page. 😄
ulisesmac
approved these changes
Dec 18, 2023
| [] | ||
| (let [status (reagent/atom :sending) | ||
| {:keys [color]} (rf/sub [:wallet/current-viewing-account])] | ||
| [floating-button-page/view |
J-Son89
force-pushed
the
jc/first-pass-transaction-page
branch
2 times, most recently
from
20e154f to
a5dd937
Compare
J-Son89
commented
Dec 20, 2023
| @@ -8,7 +8,7 @@ | |||
| [token] | |||
| (let [token-symbol (cond-> token | |||
| (keyword? token) name | |||
| :always string/lower-case)] | |||
| :always (comp string/lower-case str))] | |||
There was a problem hiding this comment.
string/lower-case crashes with nil. discussed with @ulisesmac and he came up with this approach to handle it 👌
There was a problem hiding this comment.
@J-Son89 @ulisesmac I think this needs to be reverted. Currently, no icons are displayed.
I tried passing nil to this function and it works fine. I think we have a condition (keyword? token) which will evaluate to false for nil, therefore string/lower-case wouldn't be called on a nil value.
There was a problem hiding this comment.
Thanks @OmarBasem. Spotted this yesterday with @ulisesmac and fixed in the send pr
There was a problem hiding this comment.
It was breaking with nil btw, I wouldn't have added the code otherwise 👌
smohamedjavid
approved these changes
Dec 20, 2023
J-Son89
force-pushed
the
jc/first-pass-transaction-page
branch
2 times, most recently
from
4f23891 to
520afdb
Compare
OmarBasem
approved these changes
Dec 21, 2023
J-Son89
force-pushed
the
jc/first-pass-transaction-page
branch
from
520afdb to
83a5e1e
Compare
J-Son89
force-pushed
the
jc/first-pass-transaction-page
branch
from
83a5e1e to
6b0a892
Compare
|
@status-im/mobile-qa - this pr does not add new functionality. I.e I added the base outline of some new ui but did not connect this to the app. |
Yes, e2e should be enough in this case👍 |
|
Thanks @qoqobolo, I've triggered them now (I think) - will ping you when they're done and we can check if they passed or not. |
Yep, they are now waiting their turn 
|
|
thanks @qoqobolo! 🙏 |
71% of end-end tests have passedNot executed tests (1)Failed tests (9)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Expected to fail tests (5)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (34)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestDeepLinksOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
|
@qoqobolo - how's it looking? 🤔 |
|
@J-Son89 hmm lemme rerun some of them. I'll check the results when they are done |
|
Thanks! 🙏 |
100% of end-end tests have passedPassed tests (2)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
|
@J-Son89 LGTM now :) the rest of the failures are not related to PR, you can merge it. |
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.
partially addresses: #16934
Adding a first pass of the transaction progress page so that we can piece together the whole sending flow soon and then polish off these pages afterwards. The page is added in isolation and not conencted up to the wallet flows yet as waiting on some other pr's to be merged first https://github.com/status-im/status-mobile/pull/18242/files (🐔 & 🥚 )
Implementation so far:
Testing Notes:
This pr just adds a quick outline for the UI of the transaction progress page.
I have not actually connected this page up, so this code is being added mostly for a code review as QA of the feature etc will be done in another pr when we connect the pages together.
Technically nothing new is added here, it just needs a quick pass and e2e tests (afaik)