Implement "Drawer top" component #17196
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mmilad75
requested review from
ilmotta,
J-Son89,
smohamedjavid and
OmarBasem
Jenkins BuildsClick to see older builds (32)
|
ilmotta
approved these changes
Sep 5, 2023
| (defn- subtitle | ||
| [{:keys [type theme blur? keycard? networks description community-name community-logo]}] | ||
| (cond | ||
| (= :keypair type) [rn/view {:style style/row} |
There was a problem hiding this comment.
Usually in our repo we see line breaks after the conditions to avoid right-aligning too much, otherwise it forces zprint to add line breaks too often (because its configured line length is 105 characters).
Usually when the cond body extends to a few dozen lines, it's common to add an empty line between sections of the cond macro. Side note: It's also possible to configure zprint to add line breaks between pairs in the cond macro (https://github.com/kkinnear/zprint/blob/main/doc/reference.md#map-nl-pair-nl-binding-nl).
So my suggestion here would be to style like this for improved readability:
(defn- subtitle
[{:keys [type theme blur? keycard? networks description community-name community-logo]}]
(cond
(= :keypair type)
[rn/view {:style style/row}
[text/text
{:size :paragraph-2
:weight :regular
:style (style/description theme blur?)}
(if keycard?
(i18n/label :t/on-keycard)
(i18n/label :t/on-device))]
(when keycard?
[icons/icon :i/keycard-card
{:color (colors/theme-colors colors/neutral-50 colors/neutral-40 theme)}])]
(= :account type)
[rn/view {:style style/row}
(for [network networks]
^{:key (str network)}
[network-view network])
[text/text
{:size :paragraph-2
:weight :regular
:style (style/description theme blur?)}
description]]
(= :default-keypair type)
[text/text
{:accessibility-label :default-keypair-text
:size :paragraph-2
:weight :regular
:style (style/description theme blur?)}
(str description " ∙ " (i18n/label :t/on-device))]
(= :context-tag type)
[rn/view
{:accessibility-label :context-tag-wrapper
:style {:flex-wrap :wrap}}
[context-tag/view
{:type :community
:community-name community-name
:community-logo community-logo
:size 24}]]
(and (not= :label type) description)
[text/text
{:size :paragraph-1
:weight :regular
:style (style/description theme blur?)}
description]))There was a problem hiding this comment.
Done. Very good point 👍
J-Son89
reviewed
Sep 5, 2023
|
|
||
| (defn network-text-color | ||
| [network] | ||
| {:color (get colors/networks network)}) |
There was a problem hiding this comment.
hmm, I think we should use the custom-color resolving here. Same as we are using for everything else.
Can we add this map to the custom-colors function and then use as we are doing in the rest. That way we abstract these types of colors to that mechanism and then can just adjust that at a later as needs be 👍
There was a problem hiding this comment.
i.e imo we should update colors map in the colors file to
(def colors-map
(merge {:primary {50 primary-50 ;; User can also use primary color as customisation color
60 primary-60}
:beige {50 "#CAAE93"
60 "#AA927C"}
...
customization
networks))
There was a problem hiding this comment.
so we can use (custom-colors network 50) etc 👍
There was a problem hiding this comment.
perhaps we make these maps like networks private too using def-
J-Son89
reviewed
Sep 5, 2023
| (defn- view-internal | ||
| [{:keys [title type theme description blur? community-name community-logo button-icon on-button-press | ||
| on-button-long-press | ||
| button-disabled? account-avatar-emoji account-avatar-customization-color icon-avatar |
There was a problem hiding this comment.
imo it is better to use customization-color prop here as that's all this is. There won't be separate customization-colors on different components. This is also consistent with our other quo2 components api which is nice to have in the design system components 👍
OmarBasem
reviewed
Sep 6, 2023
| :profile-picture profile-picture}] | ||
| nil)) | ||
|
|
||
| (defn- subtitle |
There was a problem hiding this comment.
This component can be broken up into smaller components
mmilad75
force-pushed
the
milad/17021
branch
from
ff75953
to
64c3538
Compare
Francesca-G
approved these changes
Sep 8, 2023
There was a problem hiding this comment.
Here's the design review, adding follow up required for minor fixes :)
mmilad75
force-pushed
the
milad/17021
branch
from
9a9e645
to
0abc639
Compare
mmilad75
force-pushed
the
milad/17021
branch
from
85e8bd3
to
2ee7942
Compare
This reverts commit c2a9f51.
Francesca-G
approved these changes
Sep 11, 2023
There was a problem hiding this comment.
Looks good :)
Please look at Mario's comment about the dot alignment
I also fixed his comment |
|
@mmilad75 why PR has been merged without e2e and manual testing (as it was requested)? |
This also touched the colors mechanism which is used all over the application. Probably would have benefitted from having a quick QA check of the app. We only skip QA for completely new design system components where no code touches any of the code being used in the live application 👍 |
0% of end-end tests have passedFailed tests (43)Click to expandClass TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
|
flexsurfer
added a commit
that referenced
this pull request
Sep 14, 2023
This reverts commit fcbe6fe.
Francesca approved it as manual-test |
|
@Francesca-G is approved from design perspective, please, check out the doc for full process: https://github.com/status-im/status-mobile/blob/develop/doc/pipeline_process.md |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #17021
Platforms