Add quo2 dApp component #17074

erikseppanen · 2023-08-21T19:41:09Z

status: ready

status-im-auto · 2023-08-21T19:47:42Z

Jenkins Builds

Click to see older builds (8)

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	`0494114`	#1	2023-08-21 19:47:41	~6 min	`android-e2e`	🤖`apk` 📲
✔️	`0494114`	#1	2023-08-21 19:47:55	~6 min	`android`	🤖`apk` 📲
✔️	`0494114`	#1	2023-08-21 19:48:13	~6 min	`ios`	📱`ipa` 📲
✔️	`0494114`	#1	2023-08-21 19:49:59	~8 min	`tests`	📄`log`

✔️	`dcf830d`	#2	2023-08-22 01:26:42	~6 min	`android-e2e`	🤖`apk` 📲
✔️	`dcf830d`	#2	2023-08-22 01:26:45	~6 min	`android`	🤖`apk` 📲
✔️	`dcf830d`	#2	2023-08-22 01:27:09	~6 min	`ios`	📱`ipa` 📲
✔️	`dcf830d`	#2	2023-08-22 01:28:48	~8 min	`tests`	📄`log`

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	`f773450`	#3	2023-08-22 21:19:16	~5 min	`android`	🤖`apk` 📲
✔️	`f773450`	#3	2023-08-22 21:20:35	~7 min	`android-e2e`	🤖`apk` 📲
✔️	`f773450`	#3	2023-08-22 21:21:20	~7 min	`ios`	📱`ipa` 📲
✔️	`f773450`	#3	2023-08-22 21:23:10	~9 min	`tests`	📄`log`

✔️	`78f7079`	#4	2023-08-23 18:23:27	~6 min	`android`	🤖`apk` 📲
✔️	`78f7079`	#4	2023-08-23 18:24:06	~6 min	`ios`	📱`ipa` 📲
✔️	`78f7079`	#4	2023-08-23 18:24:19	~7 min	`android-e2e`	🤖`apk` 📲
✔️	`78f7079`	#5	2023-08-23 19:16:22	~8 min	`tests`	📄`log`

src/quo2/components/list_items/dapp/component_spec.cljs

J-Son89 · 2023-08-21T20:01:55Z

src/quo2/components/list_items/dapp/style.cljs

+                         colors/white-opa-5
+
+                         (= state :pressed)
+                         (colors/custom-color :blue 50 5)


We should not use :blue here, we want to pass 'customization-color' as a prop. You can check many other components that have that

Yes, thank you. done.

@J-Son89 does the current implementation (using customization-color) look ok?

ok, noted your comment below. done

J-Son89 · 2023-08-21T20:05:36Z

src/quo2/components/list_items/dapp/view.cljs

+            [quo2.theme :as quo.theme]))
+
+(defn- view-internal
+  [{:keys [dapp state action blur? on-press theme]}]


Perhaps the state :pressed should just be an internal var as it's local to the component depending on when the button is pressed or not.

Could be... I copied the implementation of account list card, which passed code review last week. link

yeah but that would mean we have to handle the state of pressed externally, I don't think we want that. it probably just went unnoticed in that component you linked. the quo2 button component does this ok imo 👍

@ajayesivan's pr has recently got a similar effect to this: I think we should follow that ->
https://github.com/status-im/status-mobile/pull/17070/files#diff-aeb553e6b1e621275db78ccbbd16de99518862183c6fb9732a3e0e915326976f

@J-Son89 Sounds good!

I concur. I would just like to point this out as an example of where we make an informed tradeoff of some fidelity between Figma properties and our component properties (in this case Figma uses state = default | pressed | active).

Thanks for posting a link to an example!

Yep 100%, and if anyone has some better approach then we can go with that. Imo the reason we want to match figma is so it's easy to configure on the pages, however pressed is not something we configure but rather the component handles itself

ajayesivan · 2023-08-22T09:14:22Z

src/quo2/components/list_items/dapp/component_spec.cljs

+                :blur?               false
+                :customization-color :blue
+                :on-press            (h/mock-fn)}])
+    (h/is-truthy (h/get-by-text "coingecko.com"))))


It would be nice if you can add more tests, like on-press event and name.

Thanks for you review @ajayesivan!

done.

ajayesivan · 2023-08-22T09:20:30Z

src/quo2/components/list_items/dapp/style.cljs

+   :padding-horizontal 12
+   :padding-vertical   8
+   :border-radius      12
+   :background-color   (cond


I like putting long conditions like this into their own function.

yes, me too! done.

ajayesivan · 2023-08-22T09:27:50Z

src/status_im2/contexts/quo_preview/list_items/dapp.cljs

+(defn cool-preview
+  []
+  (let [state (reagent/atom {:dapp                {:avatar (resources/get-mock-image :coin-gecko)
+                                                   :name   "Coingecko"
+                                                   :value  "coingecko.com"}
+                             :state               :default
+                             :action              :icon
+                             :blur?               false
+                             :customization-color :blue
+                             :on-press            (fn [] (js/alert "Button pressed"))})]
+    (fn []
+      [rn/touchable-without-feedback {:on-press rn/dismiss-keyboard!}
+       [rn/view {:padding-bottom 150}
+        [rn/view {:flex 1}
+         [preview/customizer state descriptor]]
+        [rn/view
+         {:padding-vertical 60
+          :flex-direction   :row
+          :justify-content  :center}
+         [quo/dapp @state]]]])))
+
+(defn preview
+  []
+  [rn/view
+   {:background-color (colors/theme-colors colors/white
+                                           colors/neutral-95)
+    :flex             1}
+   [rn/flat-list
+    {:flex                         1
+     :keyboard-should-persist-taps :always
+     :header                       [cool-preview]
+     :key-fn                       str}]])


Thanks to Icaro! The preview boilerplate is no longer necessary. Please take a look at this PR for reference: #16996

awesome! done.

ajayesivan · 2023-08-22T09:28:26Z

src/status_im2/contexts/quo_preview/list_items/dapp.cljs

+            [status-im2.contexts.quo-preview.preview :as preview]))
+
+(def descriptor
+  [{:label   "State"


You can omit :label, it will be autogenerated from :key

nice! done.

J-Son89 · 2023-08-22T15:56:52Z

src/quo2/components/list_items/dapp/style.cljs

+                         colors/white-opa-5
+
+                         (= state :pressed)
+                         (colors/custom-color customization-color 50 5)


Yea but you probably have to have this themed too as it is 60 for dark.
E.g
(Colors/theme-colors
(Colors/custom-color ... 50 5)
(Colors/custom-color ... 60 5)
theme)

Yes, we should use 60 variants for the dark theme, but for some reason, in Figma both variants (light & dark) are using 50 variants. Not sure, but probably a design bug. Maybe @mariocnovoa can take a look.

@J-Son89 As @Parveshdhull mentioned, I followed the Figma designs. But I agree that with you both that this is probably a bug, so I updated it as you described. @mariocnovoa: if Figma is indeed correct, let me know, and I will revert this to use the same colors for both light and dark themes.

erikseppanen · 2023-08-23T01:26:52Z

@Francesca-G Appreciate your review of this component

yqrashawn

LGTM

Francesca-G

Nice job ✨

codemaster115

LGTM 👍

erikseppanen requested review from J-Son89, FFFra, yqrashawn, OmarBasem, codemaster115, ajayesivan and Parveshdhull August 21, 2023 19:44

J-Son89 reviewed Aug 21, 2023

View reviewed changes

ajayesivan assigned erikseppanen Aug 21, 2023

erikseppanen force-pushed the esep/dapp branch from 0494114 to dcf830d Compare August 22, 2023 01:20

ajayesivan reviewed Aug 22, 2023

View reviewed changes

ajayesivan mentioned this pull request Aug 22, 2023

Refactor quo channel component #17070

Merged

OmarBasem approved these changes Aug 22, 2023

View reviewed changes

J-Son89 reviewed Aug 22, 2023

View reviewed changes

erikseppanen force-pushed the esep/dapp branch from dcf830d to f773450 Compare August 22, 2023 21:13

yqrashawn approved these changes Aug 23, 2023

View reviewed changes

Francesca-G approved these changes Aug 23, 2023

View reviewed changes

codemaster115 approved these changes Aug 23, 2023

View reviewed changes

Add quo2 dApp component

78f7079

erikseppanen force-pushed the esep/dapp branch from f773450 to 78f7079 Compare August 23, 2023 18:17

erikseppanen merged commit 0552713 into develop Aug 23, 2023
2 checks passed

erikseppanen deleted the esep/dapp branch August 23, 2023 20:31

andresceballosm pushed a commit to andresceballosm/status-mobile that referenced this pull request Aug 29, 2023

Add quo2 dApp component (status-im#17074)

8185efc

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add quo2 dApp component #17074

Add quo2 dApp component #17074

erikseppanen commented Aug 21, 2023

status-im-auto commented Aug 21, 2023 •

edited

Loading

J-Son89 Aug 21, 2023

erikseppanen Aug 22, 2023

erikseppanen Aug 22, 2023

erikseppanen Aug 22, 2023

J-Son89 Aug 21, 2023

erikseppanen Aug 22, 2023

J-Son89 Aug 22, 2023

J-Son89 Aug 22, 2023 •

edited

Loading

erikseppanen Aug 22, 2023

J-Son89 Aug 22, 2023

erikseppanen Aug 22, 2023 •

edited

Loading

ajayesivan Aug 22, 2023

erikseppanen Aug 22, 2023

ajayesivan Aug 22, 2023

erikseppanen Aug 22, 2023

ajayesivan Aug 22, 2023

erikseppanen Aug 22, 2023

ajayesivan Aug 22, 2023

erikseppanen Aug 22, 2023

J-Son89 Aug 22, 2023

Parveshdhull Aug 22, 2023

erikseppanen Aug 22, 2023

erikseppanen commented Aug 23, 2023

yqrashawn left a comment

Francesca-G left a comment

codemaster115 left a comment

Add quo2 dApp component #17074

Add quo2 dApp component #17074

Conversation

erikseppanen commented Aug 21, 2023

status-im-auto commented Aug 21, 2023 • edited Loading

Jenkins Builds

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

J-Son89 Aug 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikseppanen Aug 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikseppanen commented Aug 23, 2023

yqrashawn left a comment

Choose a reason for hiding this comment

Francesca-G left a comment

Choose a reason for hiding this comment

codemaster115 left a comment

Choose a reason for hiding this comment

status-im-auto commented Aug 21, 2023 •

edited

Loading

J-Son89 Aug 22, 2023 •

edited

Loading

erikseppanen Aug 22, 2023 •

edited

Loading