Quo2 Wallet: Settings Item #17179
Quo2 Wallet: Settings Item #17179
Conversation
OmarBasem
requested review from
J-Son89,
smohamedjavid,
briansztamfater,
rasom,
tumanov-alex and
mmilad75
| 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 |
There was a problem hiding this comment.
@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.
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.
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.
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.
@J-Son89 Yeah seperate PR. This PR just fixes vertical alignment for size 24
Jenkins BuildsClick to see older builds (31)
|
| :selector [selectors/toggle action-props] | ||
| nil)]) | ||
|
|
||
| (defn- internal-view |
There was a problem hiding this comment.
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]
| @@ -61,9 +64,10 @@ | |||
| :color text-color}} label]]]))) | |||
|
|
|||
| (defn- positive | |||
| [size theme label _ no-icon?] | |||
| [size theme label _ no-icon? container-style] | |||
There was a problem hiding this comment.
preferably these args are a map instead
There was a problem hiding this comment.
This would affect the rest of the component and would need further changes for all the other cases. This is not part of this PR, I just needed to add a container-style
src/status_im2/contexts/quo_preview/settings/settings_item.cljs
Outdated
Show resolved
Hide resolved
| (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.
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
| (:require [quo2.components.settings.settings-item.view :as settings-item] | ||
| [test-helpers.component :as h])) | ||
|
|
||
| (def props |
There was a problem hiding this comment.
args for tests should ideally be the bare minimum for the test to work imo.
There was a problem hiding this comment.
trimmed it down a little
| (let [icon-height (if (= image :icon-avatar) 32 20) | ||
| icon-height (if description 40 icon-height)] |
There was a problem hiding this comment.
@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.
OmarBasem
force-pushed
the
quo2/settings-item
branch
from
376b955
to
40cd959
Compare
|
@Francesca-G a design review please :) |
|
@Francesca-G there is a known issue with letter spacing. I don't think it is specific to this PR. I double checked the values used in Figma and they are matching what I am using |
I'm talking about the icon, not the letter spacing. The icon isn't aligned with the text |
|
@Francesca-G please check now |
OmarBasem
force-pushed
the
quo2/settings-item
branch
from
7b4bf87
to
d3e9a19
Compare
|
@Francesca-G please check again :) |
74% of end-end tests have passedFailed tests (11)Click to expandClass TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePRTwo:
Passed tests (32)Click to expandClass TestCommunityOneDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePRTwo:
|
OmarBasem
force-pushed
the
quo2/settings-item
branch
2 times, most recently
from
ca865e9
to
a76ed62
Compare
OmarBasem
force-pushed
the
quo2/settings-item
branch
from
a76ed62
to
ed1d34e
Compare
|
curious, seems like this pr was not reviewed/approved by QA. However this code is touching design system components which are used in the application code. Is that the case? 🤔 |
|
@J-Son89 I was not aware that it is used in application code. Did it break anything? |
|
ah okay, yes it had some uses in the codebase. Not to worry, it's a small enough fix 👍 |
fixes: #17124
also fixes: #17125
This PR reimplements Settings item component to match figma and add the missing variations. Blur is out of scope.
Designs
Demo:
RPReplay-Final1693851039.MP4