Skip to content

New intro screen #15127

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

Merged
merged 2 commits into from
Mar 6, 2023
Merged

New intro screen #15127

merged 2 commits into from
Mar 6, 2023

Conversation

J-Son89
Copy link
Contributor

@J-Son89 J-Son89 commented Feb 17, 2023

fixes: #15014

This pr implements the basic structure of the new "intro" page.

The carousel implemented is only a quick solution with no animation, I will create a separate issue to address this and also add blur effect to the buttons. Currently we want to get this flow started so other devs work can begin as well.

Screenshot 2023-02-17 at 23 17 42

Screenshot 2023-02-17 at 23 17 38

Screenshot 2023-02-17 at 23 17 40

Screenshot 2023-02-17 at 23 17 53

Currently both buttons will just direct the user to the next page that would have been on the flow anyway.
i.e
no matter what they select they will be brought to:

Screenshot 2023-02-17 at 23 18 14

To test:
Open up the application without any saved data, you should see the new opening screen

@status-im-auto
Copy link
Member

status-im-auto commented Feb 17, 2023

Jenkins Builds

Click to see older builds (32)
Commit #️⃣ Finished (UTC) Duration Platform Result
61250c6 #1 2023-02-17 23:24:18 ~24 sec android 📄log
61250c6 #1 2023-02-17 23:24:21 ~27 sec android-e2e 📄log
61250c6 #1 2023-02-17 23:24:22 ~23 sec tests 📄log
61250c6 #1 2023-02-17 23:24:24 ~25 sec ios 📄log
61250c6 #2 2023-03-02 13:25:46 ~23 sec ios 📄log
61250c6 #2 2023-03-02 13:25:46 ~27 sec android 📄log
61250c6 #2 2023-03-02 13:25:50 ~31 sec android-e2e 📄log
61250c6 #2 2023-03-02 13:25:53 ~31 sec tests 📄log
✔️ e4b910f #3 2023-03-02 14:37:49 ~4 min tests 📄log
✔️ e4b910f #3 2023-03-02 14:41:01 ~8 min android 🤖apk 📲
✔️ e4b910f #3 2023-03-02 14:42:04 ~9 min ios 📱ipa 📲
✔️ e4b910f #3 2023-03-02 14:42:34 ~9 min android-e2e 🤖apk 📲
e07a20c #4 2023-03-02 15:55:55 ~2 min tests 📄log
✔️ e07a20c #4 2023-03-02 16:00:37 ~7 min ios 📱ipa 📲
✔️ e07a20c #4 2023-03-02 16:01:44 ~8 min android-e2e 🤖apk 📲
✔️ e07a20c #4 2023-03-02 16:02:11 ~8 min android 🤖apk 📲
✔️ d491ac2 #5 2023-03-02 17:04:52 ~5 min tests 📄log
✔️ d491ac2 #5 2023-03-02 17:05:26 ~6 min ios 📱ipa 📲
✔️ d491ac2 #5 2023-03-02 17:07:17 ~8 min android-e2e 🤖apk 📲
✔️ d491ac2 #5 2023-03-02 17:07:56 ~8 min android 🤖apk 📲
✔️ 01e33be #6 2023-03-05 21:48:19 ~4 min tests 📄log
✔️ 01e33be #6 2023-03-05 21:51:10 ~7 min android 🤖apk 📲
✔️ 01e33be #6 2023-03-05 21:51:12 ~7 min ios 📱ipa 📲
✔️ 01e33be #6 2023-03-05 21:51:43 ~8 min android-e2e 🤖apk 📲
✔️ 306c8ea #7 2023-03-05 21:58:09 ~4 min tests 📄log
✔️ 306c8ea #7 2023-03-05 22:00:40 ~7 min ios 📱ipa 📲
✔️ 306c8ea #7 2023-03-05 22:00:51 ~7 min android-e2e 🤖apk 📲
✔️ 306c8ea #7 2023-03-05 22:02:12 ~8 min android 🤖apk 📲
✔️ be49370 #8 2023-03-06 09:06:09 ~4 min tests 📄log
✔️ be49370 #8 2023-03-06 09:09:03 ~7 min ios 📱ipa 📲
✔️ be49370 #8 2023-03-06 09:09:06 ~7 min android-e2e 🤖apk 📲
✔️ be49370 #8 2023-03-06 09:10:09 ~8 min android 🤖apk 📲
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ e6a96e8 #10 2023-03-06 10:01:21 ~5 min tests 📄log
✔️ e6a96e8 #10 2023-03-06 10:03:39 ~7 min android-e2e 🤖apk 📲
✔️ e6a96e8 #10 2023-03-06 10:03:48 ~7 min ios 📱ipa 📲
✔️ e6a96e8 #10 2023-03-06 10:04:35 ~8 min android 🤖apk 📲
✔️ 6c5082b #12 2023-03-06 13:19:47 ~4 min tests 📄log
✔️ 6c5082b #12 2023-03-06 13:22:44 ~7 min ios 📱ipa 📲
✔️ 6c5082b #12 2023-03-06 13:23:34 ~8 min android-e2e 🤖apk 📲
✔️ 6c5082b #12 2023-03-06 13:24:03 ~8 min android 🤖apk 📲

@J-Son89 J-Son89 force-pushed the jc/migrate-resources branch from c76837d to 871288a Compare February 17, 2023 23:50
@J-Son89 J-Son89 self-assigned this Feb 17, 2023
@J-Son89 J-Son89 removed request for jakubgs and churik February 17, 2023 23:51
@J-Son89 J-Son89 changed the title New on boarding New intro screen Feb 18, 2023
@J-Son89 J-Son89 force-pushed the jc/migrate-resources branch from 457c10f to dc4ad63 Compare February 18, 2023 00:17
@J-Son89
Copy link
Contributor Author

J-Son89 commented Feb 18, 2023

This pr, is ready for dev review, pretty sure tests etc are failing because its base branch is pointing to my migrate resources branch.

@J-Son89 J-Son89 force-pushed the jc/migrate-resources branch 3 times, most recently from 6647ff9 to f387d59 Compare February 22, 2023 15:34
Copy link
Contributor

@ulisesmac ulisesmac left a comment

Choose a reason for hiding this comment

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

Hey @J-Son89 I added some comments :)

1500)]
#(js/clearInterval interval))))
[rn/view {:style style/page-container}

Copy link
Contributor

Choose a reason for hiding this comment

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

We have various extra empty lines, could you please remove them? :)

(defn view
[]
[:f>
(fn []
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if it's a good practice to delcare the component in this way in terms of performance 🤔
I'd say it's better to create the component using defn so it's not redefined each time view is called.

What do other devs think about it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's so I could a hook, but is it possible to use hooks with the [:f> ?

Copy link
Contributor

@ulisesmac ulisesmac Mar 2, 2023

Choose a reason for hiding this comment

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

Yes, if you use defn you can always call to that component using [:f> ,,, ] and it will support hooks

[]
[:f>
(fn []
(rn/use-effect (fn []
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about using the following pattern in this case instead of using a functional component and the use-effect hook:

(defn view
  []
  (reagent/with-let [carousel-index (reagent/atom 0)
                     interval-id    (js/setInterval
                                     #(swap! carousel-index set-index)
                                     1500)]
    [rn/view {:style style/page-container}
     ;; your view code
     ]
    (finally
     (js/clearInterval interval-id))))

I've tested it and it works as expected, plus is easier to read 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.

sure thing, I can adjust 👍

{:root {:stack {:id :intro
:children [{:component {:name :intro
:id :intro
:options (merge
Copy link
Contributor

Choose a reason for hiding this comment

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

We could use a map with two keys instead merging two maps

:drawBehind true}}
{:statusBar {:style (if dark-mode? :light :dark)}})))
([dark-root?]
(let [dark-mode? (if dark-root? (colors/dark?) (utils.theme/dark-mode?))]
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious about what was that :shell-stack.

I understand we'll pass true to this function for the onborading screens we'll create, is it correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I might need to revert this code. That said, the navigation code was a bit complicated and I did not like coupling the shell-stack to that function when it could be passed as a param.
I think ideally it could be done that way @ulisesmac but would appreciate if @Parveshdhull can check it as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

(colors/dark?) depends on the user's choice (can only be accessed after login, that's why shell-stack) and (utils.theme/dark-mode?) depends on the system theme setting.
So after login we should use the user's choice. And as we didn't had access to that before login, we were using the system theme upto now.

But for new designs, onboarding is always the dark theme, so we can use
dark-mode? (if (= @state/root-id :shell-stack) (colors/dark?) true)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reverted this change for now as it's not necessary. I think it would be nice to make this not coupled with the state and just pass it in as a param.

@@ -148,10 +139,19 @@
[]
;;TABS
(merge (old-roots)
;;INTRO (onboarding carousel)
{:intro-stack
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please explain what's the way to add more screens to this :intro-stack? I guess we should add them into that :children vector, but I'm not sure.

Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as far as I understand we should add them to src/status_im2/navigation/screens.cljs and then have a button that navigates to that screen. @Parveshdhull, that's how it should work,right?

Copy link
Contributor

@Parveshdhull Parveshdhull Mar 2, 2023

Choose a reason for hiding this comment

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

Yes, we should navigate to them with buttons.
Multiple children are used for pushing several screens at once while changing root.
For ex: After login instead of showing the shell screen, directly show chat screen. (where shell screen is also opened below it)

:lifestyle (js/require "../resources/images/ui2/lifestyle.png")
:music (js/require "../resources/images/ui2/music.png")
:podcasts (js/require "../resources/images/ui2/podcasts.png")
:intro-1 (js/require "../resources/images/ui2/intro-1.png")
Copy link
Contributor

Choose a reason for hiding this comment

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

The images added contain a transparent padding top (i.e. some transparent pixels), is it correct?
I've seen this sometimes in the designs, but I'm not sure whether that's due to the way figma exports the files or it's intended by the design team.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, let's check with design team about this 👍 I exported using Figma which is the method we normally use afaik

Copy link
Contributor Author

Choose a reason for hiding this comment

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

images look good in the app btw 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

yes they looks different images
image

Fresh Install flow uses different image then app locked etc.
(probably they are just placeholder and going to be replaced, not sure)

Copy link
Contributor

Choose a reason for hiding this comment

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

@J-Son89 @Parveshdhull I'm talking about these transparent pixels:
image

Is it ok to keep that "padding-like"?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should use this kind of images without transparent pixels, and add paddings programatically if necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll discuss with design team!

@J-Son89 J-Son89 force-pushed the jc/migrate-resources branch 8 times, most recently from 2287309 to c3c50e9 Compare February 25, 2023 18:56
@J-Son89 J-Son89 force-pushed the jc/new-onboarding branch from 306c8ea to 6322db4 Compare March 6, 2023 09:01
@J-Son89 J-Son89 force-pushed the jc/new-onboarding branch 2 times, most recently from 84d7be9 to e6a96e8 Compare March 6, 2023 09:55
@J-Son89
Copy link
Contributor Author

J-Son89 commented Mar 6, 2023

checked with design team and fixed the borders issue on Android phones
Screenshot 2023-03-06 at 09 53 18

ios/Podfile.lock Outdated
@@ -228,6 +228,8 @@ PODS:
- React-Core
- react-native-netinfo (4.7.0):
- React
- react-native-orientation-locker (1.5.0):
Copy link
Contributor

Choose a reason for hiding this comment

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

how this related to this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's not, will remove. thanks for spotting 👍

:bottom 0
:left 0
:right 0
:width "100%"
Copy link
Contributor

Choose a reason for hiding this comment

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

redundant width and height

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will double check this 👍

(def carousel
{:height 92
;; (padding-top) This insets issue needs a consistent implementation across all screens.
:padding-top (if platform/android? 0 44)
Copy link
Contributor

Choose a reason for hiding this comment

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

why is it different on android ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a common occurrence across the application. Imo it's another thing that need to be revisited in the navigation code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

or should I make use of the insets here? I believe for the redesign it is consistently like this however since we are hiding the top bar. Perhaps we can just put this in as the default inset for navigation screens and then clean up all the screens in a refactor? @Parveshdhull @flexsurfer wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes i see a lot of this in the new code, and it looks like a misconception, we shouldn't do it in every place

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay, how would you feel if I leave it as is in this pr and do a follow up to configure all the screens setting this value in one place, i.e navigation screens?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes sure it should be done in separate PR

:style style/highlighted-text
:weight :semi-bold}
(i18n/label :t/terms-of-service)]]]]
(finally (js/clearInterval interval-id))))
Copy link
Contributor

Choose a reason for hiding this comment

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

nice , TIL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

credit to @ulisesmac :)

@churik churik mentioned this pull request Mar 6, 2023
@status-im-auto
Copy link
Member

89% of end-end tests have passed

Total executed tests: 27
Failed tests: 3
Passed tests: 24
IDs of failed tests: 702855,702838,702840 

Failed tests (3)

Click to expand
  • Rerun failed tests

  • Class TestOneToOneChatMultipleSharedDevicesNewUi:

    1. test_1_1_chat_edit_message, id: 702855

    Device 1: Find `Text` by `xpath`: `//*[starts-with(@text,'AFTER')]/ancestor::android.view.ViewGroup[@content-desc='chat-item']`
    Device 1: Tap on found: Text

    critical/chats/test_1_1_public_chats.py:1274: in test_1_1_chat_edit_message
        self.errors.verify_no_errors()
    base_test_case.py:183: in verify_no_errors
        pytest.fail('\n '.join([self.errors.pop(0) for _ in range(len(self.errors))]))
     Edited message is shown for receiver with status  but it should be "Edited" 
    

    [[blocked by 15166]]

    Device sessions

    Class TestCommunityMultipleDeviceMerged:

    1. test_community_message_send_check_timestamps_sender_username, id: 702838

    Device 2: Verifying that 'hello' is under today
    Device 2: Looking for a message by text: hello

    critical/test_public_chat_browsing.py:403: in test_community_message_send_check_timestamps_sender_username
        channel.verify_message_is_under_today_text(message, self.errors)
    ../views/chat_view.py:889: in verify_message_is_under_today_text
        message_element.wait_for_visibility_of_element()
    ../views/base_element.py:134: in wait_for_visibility_of_element
        raise TimeoutException(
     Device 2: ChatElementByText by xpath:`//*[starts-with(@text,'hello')]/ancestor::android.view.ViewGroup[@content-desc='chat-item']` is not found on the screen after wait_for_visibility_of_element 
    

    [[blocked by 14797]]

    Device sessions

    2. test_community_emoji_send_copy_paste_reply, id: 702840

    Device 1: Find RepliedMessageText by xpath: //*[starts-with(@text,'test message')]/preceding::android.widget.TextView[@content-desc='quoted-message']
    Device 1: RepliedMessageText is 🤑

    critical/test_public_chat_browsing.py:467: in test_community_emoji_send_copy_paste_reply
        self.errors.verify_no_errors()
    base_test_case.py:183: in verify_no_errors
        pytest.fail('\n '.join([self.errors.pop(0) for _ in range(len(self.errors))]))
     Emoji message was not copied
    



    Device sessions

    Passed tests (24)

    Click to expand

    Class TestGroupChatMultipleDeviceMergedNewUI:

    1. test_group_chat_pin_messages, id: 702732
    Device sessions

    2. test_group_chat_join_send_text_messages_push, id: 702807
    Device sessions

    3. test_group_chat_offline_pn, id: 702808
    Device sessions

    Class TestCommunityMultipleDeviceMerged:

    1. test_community_message_delete, id: 702839
    Device sessions

    2. test_community_leave, id: 702845
    Device sessions

    3. test_community_message_edit, id: 702843
    Device sessions

    4. test_community_links_with_previews_github_youtube_twitter_gif_send_enable, id: 702844
    Device sessions

    5. test_community_unread_messages_badge, id: 702841
    Device sessions

    Class TestActivityCenterMultipleDevicePR:

    1. test_activity_center_cancel_outgoing_contact_request_no_pn, id: 702871
    Device sessions

    2. test_activity_center_decline_contact_request, id: 702850
    Device sessions

    3. test_activity_center_mentions_in_community_jump_to, id: 702851
    Device sessions

    Class TestDeeplinkOneDeviceNewUI:

    1. test_deep_link_open_user_profile, id: 702775
    Device sessions

    2. test_public_chat_open_using_deep_link, id: 702776
    Device sessions

    3. test_deep_link_with_invalid_user_public_key_own_profile_key, id: 702774
    Device sessions

    Class TestCommunityOneDeviceMerged:

    1. test_community_navigate_to_channel_when_relaunch, id: 702846
    Device sessions

    2. test_community_copy_and_paste_message_in_chat_input, id: 702742
    Device sessions

    Class TestOneToOneChatMultipleSharedDevicesNewUi:

    1. test_1_1_chat_push_emoji, id: 702813
    Device sessions

    2. test_1_1_chat_is_shown_message_sent_delivered_from_offline, id: 702783
    Device sessions

    3. test_1_1_chat_text_message_delete_push_disappear, id: 702733
    Device sessions

    4. test_1_1_chat_message_reaction, id: 702730
    Device sessions

    5. test_1_1_chat_emoji_send_reply_and_open_link, id: 702782
    Device sessions

    6. test_1_1_chat_pin_messages, id: 702731
    Device sessions

    7. test_1_1_chat_delete_via_long_press_relogin, id: 702784
    Device sessions

    8. test_1_1_chat_non_latin_messages_stack_update_profile_photo, id: 702745
    Device sessions

    @VolodLytvynenko
    Copy link
    Contributor

    Hi @J-Son89 thank you for the fixes. Issues 1,2,3 are fixed. Will you push some extra commits? If yes, let me know and I will retest it. Otherwise, no issues from my side for the last build and PR is ready to be merged

    @churik
    Copy link
    Member

    churik commented Mar 6, 2023

    @J-Son89 thank you for PR, I've fixed the tests in #15269 - please, merge these changes in the scope of your PR.

    @J-Son89
    Copy link
    Contributor Author

    J-Son89 commented Mar 6, 2023

    Thanks @churik, will do! :)

    @J-Son89 J-Son89 force-pushed the jc/new-onboarding branch from e6a96e8 to 5034b4c Compare March 6, 2023 13:10
    @J-Son89
    Copy link
    Contributor Author

    J-Son89 commented Mar 6, 2023

    @VladimrLitvinenko, I pushed new changes. It's only removing one line where I had redundant height setting on the background image style. So it should not affect anything.
    No problem if you want to check again, but it should be safe to merge and if not we will spot it quickly and put this back in.
    Let me know if it's okay to merge or if you prefer to check again.

    @flexsurfer
    Copy link
    Contributor

    hey @J-Son89 why is there an empty space on the top of the images ?
    image

    @J-Son89 J-Son89 merged commit 2861190 into develop Mar 6, 2023
    @J-Son89 J-Son89 deleted the jc/new-onboarding branch March 6, 2023 14:42
    @J-Son89
    Copy link
    Contributor Author

    J-Son89 commented Mar 6, 2023

    hey @J-Son89 why is there an empty space on the top of the images ?

    image

    Yes @briansztamfater was asking about this too. Talked to design team and they added this so there is more space for the text.
    I will ask them to revisit the images so they we can add the padding ourselves if required or that they can stretch out the image

    @flexsurfer
    Copy link
    Contributor

    flexsurfer commented Mar 6, 2023

    Talked to design team and they added this so there is more space for the text.

    what do you mean? why can't it be done in react layout ?

    @J-Son89
    Copy link
    Contributor Author

    J-Son89 commented Mar 6, 2023

    Sorry if unclear. Yes, that's what I am saying. I will ask designers to remove the padding from the images and we can adjust it with code if it's required.

    @pedro-et
    Copy link

    @J-Son89 I sent the image to @Parveshdhull last week. regardless, it should be in Figma next to those screens if you want to export it again

    @J-Son89
    Copy link
    Contributor Author

    J-Son89 commented Mar 13, 2023

    Thanks @pedro-et, will get the updated images in the app 😀

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    No open projects
    Archived in project
    Development

    Successfully merging this pull request may close these issues.

    Onboarding - Implement initial screens with slideshow
    9 participants