-
Notifications
You must be signed in to change notification settings - Fork 985
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
New profile UI #17521
New profile UI #17521
Conversation
Hey @nguyentruongky, and thank you so much for making your first pull request in status-mobile! ❤️ Please help us make your experience better by filling out this brief questionnaire https://goo.gl/forms/uWqNcVpVz7OIopXg2 |
src/quo2/foundations/colors.cljs
Outdated
@@ -121,6 +121,11 @@ | |||
(def white-opa-90 (alpha white 0.9)) | |||
(def white-opa-95 (alpha white 0.95)) | |||
|
|||
(def magenta "#EC266C") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can remove these colors, we already have them defined. They are also a more dynamic set of colors based on the users customization-color (set in the onboarding phase)
instead when you go to use this customization-color
you should pass as
(:require [src/quo2/foundations/colors.cljs :as colors])
...
(let [{:keys [customization-color]} (rf/sub [:profile/multiaccount])]
[quo/button {
:customization-color (colors/custom-color customization-color 50 40)
}
that's what it will be for the use case you are using it for anyway where 40 is 40% opacity.
50 there is a suffix that is used in the mapping, you can see it if you trace through the custom-color function etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you search in the codebase you'll see many other examples of customization-color
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @J-Son89 it's changed.
@@ -0,0 +1,123 @@ | |||
(ns status-im2.contexts.profile.profiles.views-v2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this namespace is new anyway so there's no need for v2 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed. It was in status_im
. I just figured out it needs to be in status_im2
at last minutes.
@@ -0,0 +1,123 @@ | |||
(ns status-im2.contexts.profile.profiles.views-v2 | |||
(:require [clojure.string :as string] | |||
[quo.core :as quo] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please don't use any components or code that lives in quo. namespaces.
We strictly want to use quo2 code 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that might also be why you needed to adjust so many icon files as you are working from mostly old code :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to quo2 most of things I can. Please review again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @nguyentruongky, the changes you made look great. I would prefer to discuss with you before reviewing again as there are some ways that I should be able to help you find the correct code/components in the codebase. let's chat on discord 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @J-Son89 How can I reach out to you on discord. Just registered with username adventurous_beagle_60958
[quo.design-system.colors :as colors] | ||
[re-frame.core :as re-frame] | ||
[reagent.core :as reagent] | ||
[status-im.ui.components.copyable-text :as copyable-text] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same for the status-im code, this is all old and needs to be removed on our side. Yet for this pr you should also not need it. This code should exist elsewhere, it's quite tricky to see what you are using here so please reach out for individual questions.
in the case of [status-im.ui.components.react :as react]
it's preffered to use [react-native.core :as rn]. you'll see a lot rn/view
etc in the codebase 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copied from the original file and changed some as current implementation, so some required
I still don't understand clearly. Deleted most of them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, np. if there is some expectation or something that is unclear it's best to discuss first. I suggest reaching out over discord as it will be faster to discuss there. 👍
[status-im2.common.qr-code-viewer.view :as qr-code-viewer] | ||
[status-im2.config :as config] | ||
[utils.i18n :as i18n]) | ||
(:require-macros [status-im.utils.views :as views])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you need this macro? 🤔 what did you use it to achieve? curious but I would assume it's not necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, copied it from the original file.
|
||
(views/defview share-chat-key | ||
[] | ||
(views/letsubs [{:keys [address ens-name]} [:popover/popover] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm @flexsurfer is there better sub for @nguyentruongky to get this address & ens-name? 🤔
some screenshot or a video recording would be greatly appreciated! :) |
src/quo/components/list/item.cljs
Outdated
|
||
(defn size->container-size | ||
[size] | ||
(case size | ||
:small 52 | ||
64)) | ||
(min size 64))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems like it's wrong to me, why did you make these changes? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed. Needed to changed to smaller size to match the design.
src/quo/components/list/item.cljs
Outdated
@@ -70,29 +70,31 @@ | |||
children)) | |||
|
|||
(defn icon-column | |||
[{:keys [icon icon-bg-color icon-color size icon-container-style]}] | |||
[{:keys [icon icon-bg-color icon-color icon-size icon-container-style]}] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, imo this prop name doesn't need to be changed or is there a particular reason for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed.
src/quo/core.cljs
Outdated
@@ -16,6 +16,7 @@ | |||
|
|||
(def text text/text) | |||
(def header header/header) | |||
(def header-action header/header-action) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see other comments about using quo2 components only. Also we generally try to avoid exporting multiple components from the one file, so in general I would prefer they were separated to their own view file.
However in this case it's not necessary as I think we should avoid using this code completely 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed it.
(def radius 16) | ||
|
||
(def top-background-view | ||
{:background-color colors/magenta-opa-40 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is where you should be using the custom-color approach
so it's better here to do:
(defn top-background-view [customization-color]
{:background-color (colors/custom-color customization-color 50 40)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Changed all its appearance.
:top 0 | ||
:left 0 | ||
:height 400 | ||
:width 400 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any particular reason for 400? would:bottom 0 :right 0
as well not be better? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just want to keep the view as the solid background (magenta) on the top when pull down.
if use bottom 0, the magenta background is visible when scroll to the bottom.
|
||
(def toolbar | ||
{:padding-bottom 16 | ||
:padding-top (safe-area/get-top) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@smohamedjavid this is alright to do, or we normally get this value at the rendering of the component view in a let var? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. We usually get the safe area value from the component's let var. 👍
;; view.cljs
(let [top (safe-area/get-top)]
[rn/view {:style (style/toolbar top)}])
;; style.cljs
(defn toolbar
[top]
{:padding-bottom 16
:padding-top top
:flex-direction :row
:align-items :center
:justify-content :space-between})
:margin-bottom 64 | ||
:overflow "hidden" | ||
:border-radius radius | ||
:background-color colors/danger-50-opa-20}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imo for danger it's better to also use the colors mechanism-> i.e in this case.
(colors/custom-color :danger 50 20)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Changed.
@@ -51,7 +51,7 @@ | |||
[status-im.ui.screens.profile.contact.views :as contact] | |||
[status-im.ui.screens.profile.group-chat.views :as profile.group-chat] | |||
[status-im.ui.screens.profile.seed.views :as profile.seed] | |||
[status-im.ui.screens.profile.user.views :as profile.user] | |||
[status-im2.contexts.profile.profiles.views-v2 :as profile.user] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would be preferable if you register the new screen in status-im2.navigation.screens
namespace instead 👍
@@ -74,7 +74,12 @@ | |||
[status-im.ui.screens.wallet.settings.views :as wallet-settings] | |||
[status-im.ui.screens.wallet.swap.views :as wallet.swap] | |||
[status-im.ui.screens.wallet.transactions.views :as wallet-transactions] | |||
[status-im2.contexts.chat.group-details.view :as group-details])) | |||
[status-im2.contexts.chat.group-details.view :as group-details] | |||
[status-im2.contexts.chat.home.view :as chat-home] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's unclear why you have added all of these screens here? we already have them registered in the app? any particular reason? 🤨
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Want to navigate to the chat home screen. Just removed.
@J-Son89 added a video at https://monosnap.com/file/M7TUAbEHn8eSRCJKzWD3zW41NY7t83. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you pls rollback all icon files
|
||
[rn/view {:flex 1 :style styles/container-style} | ||
|
||
components/top-background-view |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks strange, could you elaborate?
ens-name address | ||
key-uid] | ||
:as account} | ||
@(re-frame/subscribe [:profile/multiaccount]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use rf/sub
instead of @(re-frame/subscribe
has-picture @(re-frame/subscribe [:profile/has-picture]) | ||
link (universal-links/generate-link :user :external (or ens-name address))] | ||
|
||
[rn/view {:flex 1 :style styles/container-style} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks strange, why flex outside style map?
@@ -129,6 +130,12 @@ | |||
:on-focus [:onboarding/overlay-dismiss] | |||
:component profiles/view} | |||
|
|||
{:name :my-profile | |||
:options {:theme :dark | |||
:layout options/onboarding-layout} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you elaborate on why /onboarding-layout is used?
@@ -772,7 +772,7 @@ SPEC CHECKSUMS: | |||
RNLanguages: 962e562af0d34ab1958d89bcfdb64fafc37c513e | |||
RNPermissions: ad71dd4f767ec254f2cd57592fbee02afee75467 | |||
RNReactNativeHapticFeedback: 2566b468cc8d0e7bb2f84b23adc0f4614594d071 | |||
RNReanimated: 43adb0307a62c1ce9694f36f124ca3b51a15272a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please rollback this file
@@ -80,7 +80,7 @@ | |||
:flex 1}) | |||
|
|||
(defn header-action | |||
[{:keys [icon label on-press disabled accessibility-label]}] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please rollback this file
@@ -90,9 +90,11 @@ | |||
(defn title-column | |||
[{:keys [title text-color subtitle subtitle-max-lines subtitle-secondary | |||
title-accessibility-label size text-size title-text-weight | |||
title-style |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please rollback this file
@@ -530,6 +526,11 @@ | |||
:options {:insets {:bottom? true | |||
:top? true}} | |||
:component bookmarks/new-bookmark} | |||
{:name :browser |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please rollback this file
:on-switch-profile nil | ||
:on-show-qr on-share}] | ||
|
||
[rn/scroll-view |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better to use flat-list
stale |
Summary
Review notes
Jump to
floating button.TODO
tags for missing screensFunctional
Before and after screenshots comparison
status: ready