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: Color picker #17405

Merged
merged 10 commits into from
Sep 27, 2023
Merged

Quo2: Color picker #17405

merged 10 commits into from
Sep 27, 2023

Conversation

OmarBasem
Copy link
Contributor

@OmarBasem OmarBasem commented Sep 25, 2023

fixes: #17122, #16584

This PR does the following updates to the color picker component

  • Updated color picker component to the modified design
  • Extracts color component to a separate component in quo2 to match Figma
  • Implements the "Feng Shui" (black & white) color circle
  • A few updates to color picker props to better match props on Figma

Desgins

Demo:

Screen_Recording_20230925_120512_Status.Debug.mp4

@status-im-auto
Copy link
Member

status-im-auto commented Sep 25, 2023

Jenkins Builds

Click to see older builds (12)
Commit #️⃣ Finished (UTC) Duration Platform Result
886b358 #1 2023-09-25 08:23:23 ~3 min tests 📄log
886b358 #1 2023-09-25 08:23:28 ~3 min ios 📄log
✔️ 886b358 #1 2023-09-25 08:26:48 ~6 min android-e2e 🤖apk 📲
✔️ 886b358 #1 2023-09-25 08:26:54 ~6 min android 🤖apk 📲
✔️ 9ab6b97 #2 2023-09-25 08:52:47 ~6 min android-e2e 🤖apk 📲
✔️ 9ab6b97 #2 2023-09-25 08:52:53 ~6 min android 🤖apk 📲
9ab6b97 #2 2023-09-25 08:55:47 ~9 min tests 📄log
✔️ 9ab6b97 #2 2023-09-25 08:56:23 ~9 min ios 📱ipa 📲
✔️ 7070bcd #3 2023-09-25 11:54:08 ~5 min android 🤖apk 📲
✔️ 7070bcd #3 2023-09-25 11:54:16 ~6 min android-e2e 🤖apk 📲
✔️ 7070bcd #3 2023-09-25 11:56:31 ~8 min ios 📱ipa 📲
✔️ 7070bcd #3 2023-09-25 11:57:58 ~9 min tests 📄log
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ decc680 #5 2023-09-26 07:20:58 ~5 min android 🤖apk 📲
✔️ decc680 #5 2023-09-26 07:21:37 ~6 min ios 📱ipa 📲
✔️ decc680 #5 2023-09-26 07:23:16 ~7 min android-e2e 🤖apk 📲
✔️ decc680 #5 2023-09-26 07:24:48 ~9 min tests 📄log
✔️ f96233c #7 2023-09-27 11:28:37 ~5 min android-e2e 🤖apk 📲
✔️ f96233c #7 2023-09-27 11:28:44 ~5 min android 🤖apk 📲
✔️ f96233c #7 2023-09-27 11:29:06 ~6 min ios 📱ipa 📲
✔️ f96233c #7 2023-09-27 11:32:34 ~9 min tests 📄log

Copy link
Contributor

@ajayesivan ajayesivan left a 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?

@OmarBasem
Copy link
Contributor Author

OmarBasem commented Sep 25, 2023

@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

src/quo2/components/colors/color/view.cljs Outdated Show resolved Hide resolved
src/quo2/components/colors/color_picker/view.cljs Outdated Show resolved Hide resolved
src/quo2/components/colors/color_picker/view.cljs Outdated Show resolved Hide resolved
src/quo2/components/colors/color/view.cljs Outdated Show resolved Hide resolved
Copy link
Contributor

@ajayesivan ajayesivan left a 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"
Copy link
Member

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.

Copy link
Contributor Author

@OmarBasem OmarBasem Sep 26, 2023

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?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

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? 🤔

@OmarBasem
Copy link
Contributor Author

@Francesca-G a design review :)

@status-im-auto
Copy link
Member

70% of end-end tests have passed

Total executed tests: 43
Failed tests: 13
Passed tests: 30
IDs of failed tests: 702809,702732,702957,703086,702894,702783,703503,702936,702786,702947,702731,702808,702958 

Failed tests (13)

Click to expand
  • Rerun failed tests

  • Class TestCommunityMultipleDeviceMergedTwo:

    1. test_community_markdown_support, id: 702809

    Device 1: Looking for a message by text: quote reply (one row)
    Device 2: Looking for a message by text: quote reply (one row)

    critical/test_public_chat_browsing.py:1201: in test_community_markdown_support
        self.errors.verify_no_errors()
    base_test_case.py:191: in verify_no_errors
        pytest.fail('\n '.join([self.errors.pop(0) for _ in range(len(self.errors))]))
     bold text in asterics is not displayed with markdown in community channel for the recipient (device 1) 
    E   
    E    bold text in underscores is not displayed with markdown in community channel for the recipient (device 1)
    



    Device sessions

    2. test_community_mentions_push_notification, id: 702786

    # STEP: Invited member gets push notification with the mention and tap it
    Device 2: Getting PN by 'user_2'

    critical/test_public_chat_browsing.py:1149: in test_community_mentions_push_notification
        self.errors.verify_no_errors()
    base_test_case.py:191: in verify_no_errors
        pytest.fail('\n '.join([self.errors.pop(0) for _ in range(len(self.errors))]))
     Push notification with the mention was not received by admin
    E    Can not edit a message with a mention
    E    Push notification with the mention was not received by the invited member 
    

    [[Issue with username in PN, issue #6 in 15500]]

    Device sessions

    Class TestActivityMultipleDevicePRTwo:

    1. test_activity_center_mentions, id: 702957

    Device 2: Looking for a message by text: open community
    Device 2: Find BaseElement by xpath: //*[starts-with(@text,'open community')]/ancestor::android.view.ViewGroup[@content-desc='chat-item']//*[@text='View']

    Test setup failed: medium/test_activity_center.py:323: in prepare_devices
        self.chat_2.chat_element_by_text(self.community_name).view_community_button.click()
    ../views/base_element.py:90: in click
        self.find_element().click()
    ../views/base_element.py:79: in find_element
        raise NoSuchElementException(
     Device 2: BaseElement by xpath: `//*[starts-with(@text,'open community')]/ancestor::android.view.ViewGroup[@content-desc='chat-item']//*[@text='View']` is not found on the screen; For documentation on this error, please visit: https://www.selenium.dev/documentation/webdriver/troubleshooting/errors#no-such-element-exception
    



    Device sessions

    2. test_activity_center_admin_notification_accept_swipe, id: 702958

    Test setup failed: medium/test_activity_center.py:323: in prepare_devices
        self.chat_2.chat_element_by_text(self.community_name).view_community_button.click()
    ../views/base_element.py:90: in click
        self.find_element().click()
    ../views/base_element.py:79: in find_element
        raise NoSuchElementException(
     Device 2: BaseElement by xpath: `//*[starts-with(@text,'open community')]/ancestor::android.view.ViewGroup[@content-desc='chat-item']//*[@text='View']` is not found on the screen; For documentation on this error, please visit: https://www.selenium.dev/documentation/webdriver/troubleshooting/errors#no-such-element-exception
    



    Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:

    1. test_1_1_chat_is_shown_message_sent_delivered_from_offline, id: 702783

    Device 2: Find Text by xpath: //*[starts-with(@text,'test message')]/ancestor::android.view.ViewGroup[@content-desc='chat-item']//*[@content-desc='message-status']/android.widget.TextView
    Device 2: Text is Sent

    critical/chats/test_1_1_public_chats.py:1416: in test_1_1_chat_is_shown_message_sent_delivered_from_offline
        self.errors.verify_no_errors()
    base_test_case.py:191: in verify_no_errors
        pytest.fail('\n '.join([self.errors.pop(0) for _ in range(len(self.errors))]))
     Message status was not changed to Delivered, it's Sent after back up online!
    



    Device sessions

    Class TestCommunityMultipleDeviceMerged:

    1. test_community_mark_all_messages_as_read, id: 703086

    Device 1: Tap on found: Button
    Device 1: Click until Text by accessibility id: community-description-text will be presented

    critical/test_public_chat_browsing.py:1028: in test_community_mark_all_messages_as_read
        self.errors.verify_no_errors()
    base_test_case.py:191: in verify_no_errors
        pytest.fail('\n '.join([self.errors.pop(0) for _ in range(len(self.errors))]))
     New messages counter is not shown in home > Community element
    E    New messages counter is not shown in community channel element
    



    Device sessions

    2. test_community_contact_block_unblock_offline, id: 702894

    Device 1: Click until ChatMessageInput by accessibility id: chat-message-input will be presented
    Device 1: Looking for a message by text: Hurray! unblocked

    critical/test_public_chat_browsing.py:967: in test_community_contact_block_unblock_offline
        self.errors.verify_no_errors()
    base_test_case.py:191: in verify_no_errors
        pytest.fail('\n '.join([self.errors.pop(0) for _ in range(len(self.errors))]))
     Hurray! unblocked was not received in public chat after user unblock! 
    

    [[Message can be missed after unblock: https://github.com//issues/16873]]

    Device sessions

    Class TestGroupChatMultipleDeviceMergedNewUI:

    1. test_group_chat_pin_messages, id: 702732

    Test is not run, e2e blocker  
    

    [[reason: [NOTRUN] Pin feature is in development]]

    2. test_group_chat_offline_pn, id: 702808

    Device 3: Looking for a message by text: message from old member
    Device 3: Looking for a message by text: message from new member

    critical/chats/test_group_chat.py:442: in test_group_chat_offline_pn
        self.errors.verify_no_errors()
    base_test_case.py:191: in verify_no_errors
        pytest.fail('\n '.join([self.errors.pop(0) for _ in range(len(self.errors))]))
     Messages PN was not fetched from offline
    



    Device sessions

    Class TestOneToOneChatMultipleSharedDevicesNewUi:

    1. test_1_1_chat_pin_messages, id: 702731

    Test is not run, e2e blocker  
    

    [[reason: [NOTRUN] Pin feature is in development]]

    Class TestCommunityOneDeviceMerged:

    1. test_community_discovery, id: 703503

    Test is not run, e2e blocker  
    

    [[reason: [NOTRUN] https://github.com//issues/17175]]

    Class TestActivityMultipleDevicePR:

    1. test_navigation_jump_to, id: 702936

    Device 2: Looking for a message by text: open community
    Device 2: Find BaseElement by xpath: //*[starts-with(@text,'open community')]/ancestor::android.view.ViewGroup[@content-desc='chat-item']//*[@text='View']

    Test setup failed: medium/test_activity_center.py:175: in prepare_devices
        self.chat_2.chat_element_by_text(self.community_name).view_community_button.click()
    ../views/base_element.py:90: in click
        self.find_element().click()
    ../views/base_element.py:79: in find_element
        raise NoSuchElementException(
     Device 2: BaseElement by xpath: `//*[starts-with(@text,'open community')]/ancestor::android.view.ViewGroup[@content-desc='chat-item']//*[@text='View']` is not found on the screen; For documentation on this error, please visit: https://www.selenium.dev/documentation/webdriver/troubleshooting/errors#no-such-element-exception
    



    Device sessions

    2. test_activity_center_reply_read_unread_delete_filter_swipe, id: 702947

    Test setup failed: medium/test_activity_center.py:175: in prepare_devices
        self.chat_2.chat_element_by_text(self.community_name).view_community_button.click()
    ../views/base_element.py:90: in click
        self.find_element().click()
    ../views/base_element.py:79: in find_element
        raise NoSuchElementException(
     Device 2: BaseElement by xpath: `//*[starts-with(@text,'open community')]/ancestor::android.view.ViewGroup[@content-desc='chat-item']//*[@text='View']` is not found on the screen; For documentation on this error, please visit: https://www.selenium.dev/documentation/webdriver/troubleshooting/errors#no-such-element-exception
    



    Passed tests (30)

    Click to expand

    Class TestCommunityMultipleDeviceMergedTwo:

    1. test_community_hashtag_links_to_community_channels, id: 702948
    Device sessions

    2. test_community_leave, id: 702845
    Device sessions

    Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:

    1. test_1_1_chat_delete_via_long_press_relogin, id: 702784
    Device sessions

    2. test_1_1_chat_mute_chat, id: 703496
    Device sessions

    Class TestActivityCenterContactRequestMultipleDevicePR:

    1. test_activity_center_contact_request_accept_swipe_mark_all_as_read, id: 702851
    Device sessions

    2. test_activity_center_contact_request_decline, id: 702850
    Device sessions

    Class TestCommunityMultipleDeviceMerged:

    1. test_community_several_images_send_reply, id: 703194
    Device sessions

    2. test_community_one_image_send_reply, id: 702859
    Device sessions

    3. test_community_emoji_send_copy_paste_reply, id: 702840
    Device sessions

    4. test_community_message_delete, id: 702839
    Device sessions

    5. test_community_message_send_check_timestamps_sender_username, id: 702838
    Device sessions

    6. test_community_links_with_previews_github_youtube_twitter_gif_send_enable, id: 702844
    Device sessions

    7. test_community_message_edit, id: 702843
    Device sessions

    8. test_community_unread_messages_badge, id: 702841
    Device sessions

    Class TestGroupChatMultipleDeviceMergedNewUI:

    1. test_group_chat_mute_chat, id: 703495
    Device sessions

    2. test_group_chat_send_image_save_and_share, id: 703297
    Device sessions

    3. test_group_chat_reactions, id: 703202
    Device sessions

    4. test_group_chat_join_send_text_messages_push, id: 702807
    Device sessions

    Class TestOneToOneChatMultipleSharedDevicesNewUi:

    1. test_1_1_chat_emoji_send_reply_and_open_link, id: 702782
    Device sessions

    2. test_1_1_chat_text_message_delete_push_disappear, id: 702733
    Device sessions

    3. test_1_1_chat_push_emoji, id: 702813
    Device sessions

    4. test_1_1_chat_non_latin_messages_stack_update_profile_photo, id: 702745
    Device sessions

    5. test_1_1_chat_edit_message, id: 702855
    Device sessions

    6. test_1_1_chat_send_image_save_and_share, id: 703391
    Device sessions

    7. test_1_1_chat_message_reaction, id: 702730
    Device sessions

    Class TestCommunityOneDeviceMerged:

    1. test_restore_multiaccount_with_waku_backup_remove_switch, id: 703133
    Device sessions

    2. test_community_copy_and_paste_message_in_chat_input, id: 702742
    Device sessions

    3. test_community_undo_delete_message, id: 702869
    Device sessions

    4. test_community_navigate_to_channel_when_relaunch, id: 702846
    Device sessions

    5. test_community_mute_community_and_channel, id: 703382
    Device sessions

    Copy link

    @Francesca-G Francesca-G left a 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 :)

    @qoqobolo
    Copy link
    Contributor

    Hey https://github.com/orgs/status-im/teams/mobile-qa, can somebody check this please? An account which uses color pink already would it be affected by this PR's build?

    Thanks for the ping @OmarBasem, I'll check it shortly and get back to you with a feedback

    @qoqobolo qoqobolo self-assigned this Sep 27, 2023
    @qoqobolo
    Copy link
    Contributor

    @OmarBasem @ajayesivan yes, it doesn't work indeed.
    The color just does not appear when upgrading the version.

    However, upgrade cases are not critical right now, so I don't think it will be a big issue.
    But if there is a simple solution, it would be nice to fix this, up to you.

    Screenshot 2023-09-27 at 10 33 37

    @OmarBasem
    Copy link
    Contributor Author

    OmarBasem commented Sep 27, 2023

    Thanks for the update @qoqobolo !

    I have opened an issue in status-go status-im/status-go#4067

    @OmarBasem OmarBasem merged commit 77733b3 into develop Sep 27, 2023
    6 checks passed
    @OmarBasem OmarBasem deleted the quo2/color-picker branch September 27, 2023 11:34
    - `on-change` Callback called when a color is selected `(fn [color-name])`.
    - `blur?` Boolean to enable blur background support.}"
    [{:keys [default-selected?]}]
    Copy link
    Contributor

    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.
    Copy link
    Contributor

    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 👍

    Copy link
    Contributor Author

    @OmarBasem OmarBasem Sep 28, 2023

    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)

    Copy link
    Contributor

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

    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

    Copy link
    Contributor Author

    @OmarBasem OmarBasem Sep 28, 2023

    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

    ibrkhalil pushed a commit that referenced this pull request Oct 1, 2023
    * Quo2: color picker
    clauxx pushed a commit that referenced this pull request Oct 3, 2023
    * Quo2: color picker
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    Archived in project
    Archived in project
    Development

    Successfully merging this pull request may close these issues.

    Update Color Picker component
    8 participants