-
Notifications
You must be signed in to change notification settings - Fork 984
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
Quo2 Wallet: Settings Item #17179
Quo2 Wallet: Settings Item #17179
Changes from all commits
b19789a
cdffbee
a609b89
6365d49
528401a
55bb77a
a45c989
cd5c9bc
0ea99ff
be579a2
1b921fb
ed1d34e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
(ns quo2.components.settings.settings-item.component-spec | ||
(:require [quo2.components.settings.settings-item.view :as settings-item] | ||
[test-helpers.component :as h])) | ||
|
||
(def props | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. args for tests should ideally be the bare minimum for the test to work imo. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. trimmed it down a little |
||
{:title "Account" | ||
:accessibility-label :settings-item | ||
:action :arrow | ||
:image :icon | ||
:image-props :i/browser}) | ||
|
||
(h/describe "Settings list tests" | ||
(h/test "Default render of Setting list component" | ||
(h/render [settings-item/view props]) | ||
(h/is-truthy (h/get-by-label-text :settings-item))) | ||
|
||
(h/test "It renders a title" | ||
(h/render [settings-item/view props]) | ||
(h/is-truthy (h/get-by-text "Account"))) | ||
|
||
(h/test "its gets passed an on press event" | ||
(let [event (h/mock-fn)] | ||
(h/render [settings-item/view | ||
(merge props {:on-press event})]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit, let's use assoc There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. using |
||
(h/fire-event :press (h/get-by-text "Account")) | ||
(h/was-called event))) | ||
|
||
(h/test "on change event gets fired for toggle" | ||
(let [on-change (h/mock-fn)] | ||
(h/render [settings-item/view | ||
(merge props | ||
{:action :selector | ||
:action-props {:on-change on-change}})]) | ||
(h/fire-event :press (h/get-by-label-text :toggle-off)) | ||
(h/was-called on-change))) | ||
|
||
(h/test "It renders a label" | ||
(h/render [settings-item/view (merge props {:label :color})]) | ||
(h/is-truthy (h/get-by-label-text :label-component))) | ||
|
||
(h/test "It renders a status tag component" | ||
(h/render [settings-item/view | ||
(merge props | ||
{:tag :context | ||
:tag-props {:context "Test Tag" | ||
:icon :i/placeholder}})]) | ||
(h/is-truthy (h/get-by-text "Test Tag"))) | ||
|
||
(h/test "on press event gets fired for button" | ||
(let [event (h/mock-fn)] | ||
(h/render [settings-item/view | ||
(merge props | ||
{:action :button | ||
:action-props {:button-text "test button" | ||
:on-press event}})]) | ||
(h/fire-event :press (h/get-by-text "test button")) | ||
(h/was-called event)))) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
(ns quo2.components.settings.settings-item.style | ||
(:require [quo2.foundations.colors :as colors])) | ||
|
||
(defn find-icon-height | ||
[description tag image] | ||
(let [icon-height (if (= image :icon-avatar) 32 20) | ||
icon-height (if description 40 icon-height)] | ||
Comment on lines
+6
to
+7
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @briansztamfater Can you elaborate how? Here we first set the height based on what type of |
||
(if tag 72 icon-height))) | ||
|
||
(defn container | ||
[{:keys [in-card? tag]}] | ||
{:padding-horizontal 12 | ||
:padding-vertical (if in-card? 12 13) | ||
:flex-direction :row | ||
:justify-content :space-between | ||
:height (if tag 96 48)}) | ||
|
||
(def sub-container | ||
{:flex-direction :row | ||
:align-items :center}) | ||
|
||
(def left-container | ||
{:margin-left 12 | ||
:height "100%" | ||
:justify-content :center}) | ||
|
||
(defn image-container | ||
[description tag image] | ||
{:height (find-icon-height description tag image) | ||
:justify-content :flex-start}) | ||
|
||
(def status-container | ||
{:flex-direction :row | ||
:align-items :center}) | ||
|
||
(defn status-dot | ||
[online? theme] | ||
{:width 8 | ||
:height 8 | ||
:border-radius 8 | ||
:margin-right 6 | ||
:background-color (if online? | ||
(colors/theme-colors colors/success-50 colors/success-60 theme) | ||
(colors/theme-colors colors/danger-50 colors/danger-60 theme))}) | ||
|
||
(defn color | ||
[blur? theme] | ||
{:color (if blur? | ||
colors/white-opa-70 | ||
(colors/theme-colors colors/neutral-50 colors/neutral-40 theme))}) | ||
|
||
(defn label-dot | ||
[background-color] | ||
{:width 15 | ||
:height 15 | ||
:border-radius 12 | ||
:background-color background-color}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,117 @@ | ||
(ns quo2.components.settings.settings-item.view | ||
(:require | ||
[quo2.components.icon :as icon] | ||
[quo2.components.list-items.preview-list.view :as preview-list] | ||
[quo2.components.selectors.selectors.view :as selectors] | ||
[quo2.components.buttons.button.view :as button] | ||
[quo2.components.markdown.text :as text] | ||
[quo2.theme :as quo.theme] | ||
[react-native.core :as rn] | ||
[quo2.components.avatars.icon-avatar :as icon-avatar] | ||
[quo2.components.tags.status-tags :as status-tags] | ||
[quo2.components.tags.context-tag.view :as context-tag] | ||
[quo2.components.settings.settings-item.style :as style] | ||
[quo2.components.avatars.user-avatar.view :as user-avatar] | ||
[utils.i18n :as i18n])) | ||
|
||
(defn status-description | ||
[{:keys [description-props blur? theme]}] | ||
(let [{:keys [online? text]} description-props] | ||
[rn/view {:style style/status-container} | ||
[rn/view {:style (style/status-dot online? blur?)}] | ||
[text/text | ||
{:size :paragraph-2 | ||
:style (style/color blur? theme)} | ||
(if online? (i18n/label :t/online-now) text)]])) | ||
|
||
(defn text-description | ||
[{:keys [description-props blur? theme]}] | ||
(let [{:keys [text icon]} description-props] | ||
[rn/view | ||
{:style style/sub-container} | ||
[text/text | ||
{:size :paragraph-2 | ||
:style (style/color blur? theme)} | ||
text] | ||
(when icon | ||
[icon/icon icon | ||
(merge (style/color blur? theme) | ||
{:size 16 | ||
:container-style {:margin-left 4}})])])) | ||
|
||
(defn description-component | ||
[{:keys [description] :as props}] | ||
(case description | ||
:text [text-description props] | ||
:text-plus-icon [text-description props] | ||
:status [status-description props] | ||
nil)) | ||
|
||
(defn image-component | ||
[{:keys [image image-props description tag blur? theme]}] | ||
[rn/view | ||
{:style (style/image-container description tag image)} | ||
(case image | ||
:icon [icon/icon image-props (style/color blur? theme)] | ||
:avatar [user-avatar/user-avatar image-props] | ||
:icon-avatar [icon-avatar/icon-avatar image-props] | ||
nil)]) | ||
|
||
(defn tag-component | ||
[{:keys [tag tag-props]}] | ||
(case tag | ||
:positive [status-tags/status-tag | ||
{:status {:type :positive} | ||
:label (i18n/label :t/positive) | ||
:size :small | ||
:container-style {:margin-top 8}}] | ||
:context [context-tag/view | ||
(merge tag-props | ||
{:type :icon | ||
:size 24 | ||
:container-style {:margin-top 8 | ||
:align-self :flex-start}})] | ||
nil)) | ||
|
||
(defn label-component | ||
[{:keys [label label-props blur? theme]}] | ||
[rn/view {:accessibility-label :label-component} | ||
(case label | ||
:text [text/text | ||
{:style (style/color blur? theme)} | ||
label-props] | ||
:color [rn/view | ||
{:style (style/label-dot label-props)}] | ||
:preview [preview-list/view {:type (:type label-props)} (:data label-props)] | ||
nil)]) | ||
|
||
(defn action-component | ||
[{:keys [action action-props blur? theme]}] | ||
[rn/view {:style {:margin-left 12}} | ||
(case action | ||
:arrow [icon/icon :i/chevron-right (style/color blur? theme)] | ||
:button [button/button | ||
{:type :outline | ||
:size 24 | ||
:on-press (:on-press action-props)} | ||
(:button-text action-props)] | ||
:selector [selectors/toggle action-props] | ||
nil)]) | ||
|
||
(defn- internal-view | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. perhaps use
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @J-Son89 fixed 👍 |
||
[{:keys [title on-press accessibility-label] :as props}] | ||
[rn/pressable | ||
{:style (style/container props) | ||
:on-press on-press | ||
:accessibility-label accessibility-label} | ||
[rn/view {:style style/sub-container} | ||
[image-component props] | ||
[rn/view {:style style/left-container} | ||
[text/text {:weight :medium} title] | ||
[description-component props] | ||
[tag-component props]]] | ||
[rn/view {:style style/sub-container} | ||
[label-component props] | ||
[action-component props]]]) | ||
|
||
(def view (quo.theme/with-theme internal-view)) |
This file was deleted.
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.
@J-Son89 I think these paddings need to be revised. They seem redundant to me as height is already given.
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.
Im not entirely sure, I added some of these to get the text alignment correct in the outline variant. Perhaps it needs adjustment but we have to check all of the cases then. We really need some visual tests in place 🙂🙃
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 tried removing padding-top, padding-bottom, padding-left, padding-right, and it stayed centered. Height is given and
:align-items :center :justify-content :center
is used, so these extra paddings are redundant.Also, there is
horizontal-padding
given, why is there also padding-left and right?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.
So I will caveat that the button was already setup when I made recent refactors and I just cleaned the code a little but mostly did not touch styling except in the case of the outline variant as the text was not sitting correctly.
It appears the way the style code was done is that padding-horizontal and padding-left and padding-right are all set so tweaking these might have unexpected consequences. I agree there could be something off there as it is slightly confusingly in implementation. Feel free to adjust as you see fit alt but just consider that when making changes.
Perhaps we do this in a separate pr either?
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.
@J-Son89 Yeah seperate PR. This PR just fixes vertical alignment for size 24
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.
sounds good, thanks! 🙌