-
Notifications
You must be signed in to change notification settings - Fork 987
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: Color picker #17405
Quo2: Color picker #17405
Conversation
Jenkins BuildsClick to see older builds (12)
|
886b358
to
9ab6b97
Compare
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.
@OmarBasem Could you please add tests for the color
component & fix the failed test?
Nothing much to be tested in the color component. It is being tested in the color picker component |
9ab6b97
to
7070bcd
Compare
7070bcd
to
8af04cd
Compare
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.
LGTM! Nice Work @OmarBasem 🎉
@@ -215,7 +215,7 @@ | |||
60 "#CC6438"} | |||
:army {50 "#216266" | |||
60 "#1A4E52"} | |||
:pink {50 "#F66F8F" | |||
:flamingo {50 "#F66F8F" |
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 may cause issues to the existing accounts which use pink
. We might need to inform the QA about this.
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.
Hey @status-im/mobile-qa, can somebody check this please? An account which uses color pink
already would it be affected by this PR's build?
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.
We need to update in status-go
as well.
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.
looks like this list in status-go is wrong. Is there a primary color? 🤔
@Francesca-G a design review :) |
70% of end-end tests have passed
Failed tests (13)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePR:
Passed tests (30)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
|
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.
Looks good ✨
Here's the design review, adding follow up required to fix the spacing between the colors :)
Thanks for the ping @OmarBasem, I'll check it shortly and get back to you with a feedback |
@OmarBasem @ajayesivan yes, it doesn't work indeed. However, upgrade cases are not critical right now, so I don't think it will be a big issue. |
9347df3
to
f96233c
Compare
Thanks for the update @qoqobolo ! I have opened an issue in status-go status-im/status-go#4067 |
- `on-change` Callback called when a color is selected `(fn [color-name])`. | ||
- `blur?` Boolean to enable blur background support.}" | ||
[{:keys [default-selected?]}] |
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.
should default-selected
be a boolean? I thought this is just a way to set the default value ? 🤔
cc @ajayesivan
also the spec description should update to align with the use otherwise 👍
:blur? blur? | ||
:key color | ||
:color color}]) | ||
;; TODO: using :feng-shui? temporarily while b & w is being developed. |
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.
do we need this TODO? why do we have this feng-shui
prop? we had decided to just not show the black && white option in the time being until we develop it properly 👍
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.
feng-shui?
props is to show feng-shui
in quo2 only, and not the other screens (temporarily until implemented)
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.
Got it, thanks 👍
@@ -176,7 +177,7 @@ | |||
|
|||
;;;; Colors | |||
(def color-picker quo2.components.colors.color-picker.view/view) | |||
(def picker-colors quo2.components.colors.color-picker.view/picker-colors) | |||
(def color quo2.components.colors.color.view/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.
perhaps we should add a comment to this export to explain it's use, as it's slight breaking the pattern of quo2 interface.
cc @ajayesivan
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.
Are you talking about picker-colors
? I removed it
fixes: #17122, #16584
This PR does the following updates to the color picker component
Desgins
Demo:
Screen_Recording_20230925_120512_Status.Debug.mp4