Wallet: Network Preferences drawer #17710
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
OmarBasem
requested review from
briansztamfater,
smohamedjavid,
J-Son89 and
vkjr
Jenkins BuildsClick to see older builds (16)
|
J-Son89
reviewed
Oct 23, 2023
| :action :selector | ||
| :action-props {:type :checkbox}}]) | ||
|
|
||
| (defn network-preferences |
There was a problem hiding this comment.
since this is used in multiple places in the designs can we move this to "src/status_im2/contexts/wallet/common/network-preferences/view.cljs"
also it would be great if this takes an on-save callback so that we can trigger this when the button is pressed. I think the use cases will be different on the different screens where it's used
J-Son89
reviewed
Oct 23, 2023
| :title (i18n/label :t/address) | ||
| :subtitle account-address | ||
| :container-style (merge style/data-item | ||
| {:background-color (colors/theme-colors colors/neutral-2_5 |
There was a problem hiding this comment.
this looks like something is wrong in the data-item component. colors should be encapsulated within the component and set by a prop. Is there some variant of this component we can set to get these colors instead?
if not then most likely the component is incorrect and needs to be directly adjusted 👍
also can you provide theme prop to theme-colors function.
There was a problem hiding this comment.
Yes, it is not clear which prop gives the bg-color. The data item component is currently under review by the design team. I have opened an issue #17728
J-Son89
reviewed
Oct 23, 2023
| :action :selector | ||
| :action-props {:type :checkbox}}]) | ||
|
|
||
| (def networks-list |
There was a problem hiding this comment.
can you move this to temp at least for now, I have a pr to centralise this data so we can put it there for now until it's all in one location 👍
There was a problem hiding this comment.
This is actually a list of settings-props. Should I move it to temp?
There was a problem hiding this comment.
oh yeah some details are more props then "mock subs". mostly the data I'm referring to is what I have in my pr. I think it's fine to leave in either place really. whatever you think is best 👍
J-Son89
reviewed
Oct 23, 2023
|
|
||
| (defn- view-internal | ||
| [] | ||
| (let [account-name (reagent/atom "Account 1") | ||
| account-color (reagent/atom :purple) | ||
| account-emoji (reagent/atom "🍑") | ||
| account-address "0xd8dA6BF26964aF9D7eEd9e03E53415D37aA96045" |
There was a problem hiding this comment.
hmm, this is probably fine to leave there, because this will eventually be a sub for the address value. I think it's better that the bottom sheet takes in a address prop so we can pass this in dynamically here 👍 will work well for the save address screen that way too
J-Son89
reviewed
Oct 23, 2023
| @@ -41,6 +41,7 @@ | |||
| {:size 40 | |||
| :container-style style/button-container | |||
| :type button-one-type | |||
| :disabled? disabled? | |||
There was a problem hiding this comment.
would be nice to capture this in a component spec too 👍
J-Son89
reviewed
Oct 23, 2023
| {:left 0 | ||
| :right 0 | ||
| :padding-horizontal 20 | ||
| :padding-top 12 | ||
| :padding-top (if label 12 0) |
OmarBasem
force-pushed
the
wallet/network-preferences
branch
from
3d07772 to
4b4a951
Compare
OmarBasem
requested review from
J-Son89,
vkjr,
briansztamfater and
smohamedjavid
and removed request for
briansztamfater,
smohamedjavid and
vkjr
J-Son89
reviewed
Oct 24, 2023
| :subtitle address | ||
| :container-style (merge style/data-item | ||
| {:background-color (colors/theme-colors colors/neutral-2_5 | ||
| colors/neutral-90)})}] |
There was a problem hiding this comment.
let's add the theme prop in for theme-colors anyway 👌
J-Son89
approved these changes
Oct 24, 2023
73% of end-end tests have passedFailed tests (9)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Expected to fail tests (3)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Passed tests (33)Click to expandClass TestActivityMultipleDevicePRTwo:
Class TestActivityMultipleDevicePR:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
vkjr
approved these changes
Oct 24, 2023
Francesca-G
approved these changes
Oct 24, 2023
OmarBasem
force-pushed
the
wallet/network-preferences
branch
from
7b44c4c to
31e8336
Compare
That's right, this PR implements the initial UI part, functionality will be implemented in another PR |
|
Hey @churik, I see there are a few failed e2e tests other than the "expected to fail" tests |
|
@OmarBasem let me re-run, some failures indeed need investigation |
84% of end-end tests have passedFailed tests (4)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityOneDeviceMerged:
Expected to fail tests (3)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (38)Click to expandClass TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
|
|
@OmarBasem you can merge the PR, e2e failures are not related |
OmarBasem
force-pushed
the
wallet/network-preferences
branch
from
421123e to
a26156a
Compare
yevh-berdnyk
pushed a commit
that referenced
this pull request
Dec 8, 2023
* Wallet: Network Preferences drawer
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: #17709
This PR implements network preferences drawer UI. Handling networks selection is not part of this PR.
Designs
Demo:
Screen_Recording_20231023_120023_Status.Debug.mp4