Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wallet: generate new keypair UI #18045

Merged
merged 8 commits into from
Dec 7, 2023
Merged

Wallet: generate new keypair UI #18045

merged 8 commits into from
Dec 7, 2023

Conversation

OmarBasem
Copy link
Contributor

fixes: #17962

This PR implements generating new keypair flow UI (until showing the secret phrase to the user). This PR has no re-frame or status-go integration.

Designs

Demo:

Screen_Recording_20231201_164017_Status.Debug.mp4

@status-im-auto
Copy link
Member

status-im-auto commented Dec 1, 2023

Jenkins Builds

Click to see older builds (8)
Commit #️⃣ Finished (UTC) Duration Platform Result
06ea84d #1 2023-12-01 13:04:53 ~3 min tests 📄log
✔️ 06ea84d #1 2023-12-01 13:08:57 ~7 min android-e2e 🤖apk 📲
✔️ 06ea84d #1 2023-12-01 13:09:01 ~7 min android 🤖apk 📲
✔️ 06ea84d #1 2023-12-01 13:14:02 ~12 min ios 📱ipa 📲
435e098 #2 2023-12-01 13:24:22 ~2 min tests 📄log
✔️ 435e098 #2 2023-12-01 13:27:59 ~6 min android-e2e 🤖apk 📲
✔️ 435e098 #2 2023-12-01 13:28:08 ~6 min android 🤖apk 📲
✔️ 435e098 #2 2023-12-01 13:35:44 ~14 min ios 📱ipa 📲
Commit #️⃣ Finished (UTC) Duration Platform Result
b2709cf #3 2023-12-05 06:07:29 ~2 min tests 📄log
✔️ b2709cf #3 2023-12-05 06:10:55 ~6 min android-e2e 🤖apk 📲
✔️ b2709cf #3 2023-12-05 06:11:08 ~6 min ios 📱ipa 📲
✔️ b2709cf #3 2023-12-05 06:11:12 ~6 min android 🤖apk 📲
✔️ 0cd59c4 #4 2023-12-07 04:21:39 ~6 min android 🤖apk 📲
✔️ 0cd59c4 #4 2023-12-07 04:23:59 ~8 min android-e2e 🤖apk 📲
✔️ 0cd59c4 #4 2023-12-07 04:26:04 ~10 min tests 📄log
✔️ 0cd59c4 #4 2023-12-07 04:28:06 ~12 min ios 📱ipa 📲

[utils.i18n :as i18n]
[utils.re-frame :as rf]))

;; Temporary words
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we put this in the temp file as it makes it easier to know what we have to clean later? 🙏

[quo/text {:style {:margin-left 4}} item]])

(defn words-column
[words first?]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

first? is a bit misleading here. I think you want first-half? or perhaps I misunderstood :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we mark these helper components as private too?

:render-data (if first? 1 7)
:scroll-enabled false}])

(defn step-item
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should step item be private?

[{:keys [theme]}]
(let [steps [:t/backup-step-1 :t/backup-step-2 :t/backup-step-3
:t/backup-step-4]
checked (reagent/atom {:0 false :1 false :2 false :3 false})
Copy link
Contributor

@J-Son89 J-Son89 Dec 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imo it's easier to read if you align the map keys

(reagent/atom  
  {:0 false
   :1 false 
   :2 false 
   :3 false})

:scroll-enabled false}])

(defn step-item
[item index _ checked]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we should add "?", i.e checked?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

item is just a label in this case? maybe it's a clearer name?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

step-item in every step component (checkbox + text) under "How to backup your recovery phrase"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant the first argument here btw, "item", since the use of it is just a label it might be clearer of the use if you change it to that. (defn step-item [label index _ checked]


(defn- view-internal
[{:keys [theme]}]
(let [steps [:t/backup-step-1 :t/backup-step-2 :t/backup-step-3
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps step-labels is clearer?

@@ -8,6 +8,24 @@
[utils.i18n :as i18n]
[utils.re-frame :as rf]))

(defn keypair-options
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we should put this in a separate options file?
e.g src/status_im2/contexts/wallet/create_account/select_keypair/options.cljs

Copy link
Contributor Author

@OmarBasem OmarBasem Dec 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

select_keypair/options.cljs

@J-Son89 I don't think we should be rendering views in files named other than view.

Also, I think this is a small component to put in a separate namespace, and is not reused in other screens.

@OmarBasem OmarBasem force-pushed the wallet/new-keypair-ui branch from 435e098 to b2709cf Compare December 5, 2023 06:04
@OmarBasem OmarBasem requested review from J-Son89, smohamedjavid and vkjr and removed request for vkjr December 5, 2023 06:57
@OmarBasem
Copy link
Contributor Author

@J-Son89 please rereview 🙏

@@ -0,0 +1,5 @@
{:lint-as
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revert

Copy link

@Francesca-G Francesca-G left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's the design review :)

@OmarBasem OmarBasem force-pushed the wallet/new-keypair-ui branch from b2709cf to 0cd59c4 Compare December 7, 2023 04:15
@OmarBasem OmarBasem merged commit dc587e4 into develop Dec 7, 2023
6 checks passed
@OmarBasem OmarBasem deleted the wallet/new-keypair-ui branch December 7, 2023 05:43
yevh-berdnyk pushed a commit that referenced this pull request Dec 8, 2023
* feat: new keypair flow ui
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Archived in project
Development

Successfully merging this pull request may close these issues.

Wallet: Generating new Keypair flow
4 participants