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

Quo2 Wallet: Settings Item #17179

Merged
merged 12 commits into from
Sep 7, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/quo2/components/buttons/button/style.cljs
Original file line number Diff line number Diff line change
Expand Up @@ -85,14 +85,14 @@
56 0
40 (if border-color 8 9)
32 (if border-color 4 5)
24 (if border-color 0 1)
24 0
(if border-color 8 9)))
:padding-bottom (when-not (or icon-only? icon-left icon-right)
(case size
56 0
40 9
32 5
24 4
24 0
Comment on lines 85 to +95
Copy link
Contributor Author

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.

Copy link
Contributor

@J-Son89 J-Son89 Sep 4, 2023

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 🙂🙃

Copy link
Contributor Author

@OmarBasem OmarBasem Sep 5, 2023

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?

Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds good, thanks! 🙌

9))
:overflow :hidden
:background-color background-color
Expand Down
4 changes: 2 additions & 2 deletions src/quo2/components/settings/category/settings/view.cljs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
(ns quo2.components.settings.category.settings.view
(:require
[quo2.components.markdown.text :as text]
[quo2.components.settings.settings-list.view :as settings-list]
[quo2.components.settings.settings-item.view :as settings-item]
[quo2.foundations.colors :as colors]
[react-native.blur :as blur]
[react-native.core :as rn]
Expand All @@ -21,7 +21,7 @@
[rn/flat-list
{:data data
:style (style/settings-items theme blur?)
:render-fn (fn [item] [settings-list/settings-list item])
:render-fn (fn [item] [settings-item/view item])
:separator [rn/view {:style (style/settings-separator theme blur?)}]}]])

(def settings-category (quo.theme/with-theme category-internal))
57 changes: 57 additions & 0 deletions src/quo2/components/settings/settings_item/component_spec.cljs
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
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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})])
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, let's use assoc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

using assoc for this particular one is fine, but merge is used multiple times in this file to merge maps, so for consistency I think we can keep it merge

(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))))
57 changes: 57 additions & 0 deletions src/quo2/components/settings/settings_item/style.cljs
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
Copy link
Member

Choose a reason for hiding this comment

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

Maybe use cond here

Copy link
Contributor Author

@OmarBasem OmarBasem Sep 5, 2023

Choose a reason for hiding this comment

The 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 image. Then we add more height based on description. Then we may add more height based on tag.

(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})
117 changes: 117 additions & 0 deletions src/quo2/components/settings/settings_item/view.cljs
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
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps use :as props and pass it along and destructure as you need.

(defn- internal-view
  [{:keys [title image image-props description description-props action action-props label label-props
           tag tag-props on-press in-card? blur? accessibility-label theme] :as props}]
           
  [rn/pressable
   {:style               (style/container in-card? tag)
    :on-press            on-press
    :accessibility-label accessibility-label}
   [rn/view {:style style/sub-container}
    [image-component props]
    [rn/view {:style {:margin-left 12}}
     [text/text {:weight :medium} title]
     [description-component props]
     [tag-component props]]]
   [rn/view {:style style/sub-container}
    [label-component props]

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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))
58 changes: 0 additions & 58 deletions src/quo2/components/settings/settings_list/component_spec.cljs

This file was deleted.

Loading