feat: add identifiers screen to onboarding-flow #15684
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
J-Son89
requested review from
flexsurfer,
ilmotta,
Parveshdhull,
briansztamfater,
smohamedjavid,
jo-mut and
ulisesmac
J-Son89
commented
Apr 18, 2023
| @@ -34,21 +31,21 @@ | |||
| paused))) | |||
|
|
|||
| (defn initialize-animation | |||
| [] | |||
| [progress paused] | |||
There was a problem hiding this comment.
refactored carousel so we can use it on any page now 👍
There was a problem hiding this comment.
I don't see much benefit in doing this. This complicates stuff instead of simplifying and not sure we even have several slide animations like this in the app. I would recommend just-copy pasting the carousel code and modifying it for your use case.
There was a problem hiding this comment.
Generally speaking I think what @J-Son89 did is the correct move. We should avoid global state outside components. It's a common discussion I've seen in many other PRs, perhaps a guideline should added.
There was a problem hiding this comment.
It's a common discussion I've seen in many other PRs, perhaps a guideline should added
Maybe we create a file with name general recommendations and suggestions and move some of the stuff there (the guideline sounds a little strong)
Having local atoms is either always not feasible or sometimes makes things complicated and/or creates problems. As in this case, the problems it creates or might create are mentioned in detail in the below comment and original implementation.
I agree with you, components should have a local state. Still, we have several global atoms because they are simpler(and sometimes better) solutions.
There was a problem hiding this comment.
Probably @J-Son89 will find a solution for using a local atom in this case and solve the all above-mentioned problem, But is it worth the time he will spend in the process?
There was a problem hiding this comment.
Yes I will explore if it's possible to use a local solution. In the case that it works out it then means we have a reusable carousel component.
In which case the effort is then worth it as it can be used on any future screens.
Currently there are already 2(afaik), so it would be better than copy/pasting code when majority of this can be shared across these components, imo at least 90% (animation set-up/code, styling etc) of this can be reused without it being complicated 👍
There was a problem hiding this comment.
Good points @Parveshdhull, some recommendations might be too strong sometimes.
There was a problem hiding this comment.
So I got this working, For the background I made use of global atoms however I put these in the background file as these are the backgrounds carousel atoms. For the Identifiers page I used local atoms. Everything else remains pure functions then so the underlying functionality is completely reusable.
Basically if a page now wants to use a carousel it has to handle the setup, i.e ratoms etc and then initialise and render the carousel.
Jenkins BuildsClick to see older builds (33)
|
J-Son89
force-pushed
the
jc/refactor-carousel
branch
2 times, most recently
from
8c6e994
to
984ee14
Compare
J-Son89
commented
Apr 18, 2023
| @@ -222,6 +223,7 @@ def create_user(self, password=common_password, keycard=False, enable_notificati | |||
| # self.create_password_input.set_value(password) | |||
| # self.confirm_your_password_input.set_value(password) | |||
| # self.next_button.click() | |||
| self.identifiers_button.wait_and_click(30) | |||
There was a problem hiding this comment.
@churik, let me know if this is all that's needed for the tests
62% of end-end tests have passedFailed tests (11)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Passed tests (18)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePR:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
|
ajayesivan
reviewed
Apr 19, 2023
| (defn background-image | ||
| [content-width] | ||
| [rn/image | ||
| {:style {:resize-mode :stretch |
There was a problem hiding this comment.
This should be in style file, right?
There was a problem hiding this comment.
if its small its fine to have it here
|
|
||
| (def card-style | ||
| {:margin-horizontal 20 | ||
| :margin-bottom :auto}) |
There was a problem hiding this comment.
File should end with a new line, right?
There was a problem hiding this comment.
@ajayesivan, this one ends with a newline character. I know, it's confusing, but having a newline character is different than having an extra empty newline.
This SO is very useful https://stackoverflow.com/questions/729692/why-should-text-files-end-with-a-newline
Because that’s how the POSIX standard defines a line https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_206:
3.206 Line
A sequence of zero or more non- characters plus a terminating character.Therefore, lines not ending in a newline character aren't considered actual lines. That's why some programs have problems processing the last line of a file if it isn't newline terminated.
| {:accessibility-label :skip-identifiers | ||
| :on-press #(rf/dispatch [:navigate-to :enable-notifications]) | ||
| :override-background-color colors/white-opa-5 | ||
| :style {:justify-self :flex-end |
There was a problem hiding this comment.
Move styles to style file.
| :override-background-color colors/white-opa-5 | ||
| :style {:justify-self :flex-end | ||
| :margin-bottom 46 | ||
| :margin-horizontal 20}} (i18n/label :t/skip)]]]]))])) |
There was a problem hiding this comment.
Good catch, it's in the style guide https://guide.clojure.style/#vertically-align-fn-args.
| {:flex 1}) | ||
|
|
||
| (def content-container | ||
| {:position :absolute |
There was a problem hiding this comment.
Any particular reason for using absolute position & top instead of going with flexbox?
There was a problem hiding this comment.
here it's because it's just working with the view component for carousel. I can try refactor so it always sits below the carousel component. This was just a faster approach to get it set up
flexsurfer
reviewed
Apr 19, 2023
|
|
||
| (def content-container | ||
| {:position :absolute | ||
| :width "100%" |
There was a problem hiding this comment.
:right 0 should be used instead
flexsurfer
reviewed
Apr 19, 2023
| paused (reagent/atom nil)] | ||
| [:f> | ||
| (fn [] | ||
| (carousel.animation/initialize-animation progress paused) |
There was a problem hiding this comment.
why do we still want to init-animation? without animate? condition cc @Parveshdhull
There was a problem hiding this comment.
Yes, we should not do that. Now all the screen will have their own animations.
There was a problem hiding this comment.
this was just a misstep, have put this back in 👍
| (carousel.animation/initialize-animation progress paused) | ||
| (fn [] | ||
| [:<> | ||
| [background/view true] |
There was a problem hiding this comment.
This looks weird, background also has carousel and this screen also adds it.
There was a problem hiding this comment.
yes but this is how the page is expected to be, i.e the onboarding background uses the carousel and this page also uses a carousel and the onboarding background :)
Comment on lines
31
to
33
| (let [view-id (rf/sub [:view-id]) | ||
| progress (reagent/atom nil) | ||
| paused (reagent/atom nil)] |
There was a problem hiding this comment.
Hi @J-Son89, Having view-id, progress and paused in background file itself will not work.
- Carousel will not keep its animated state, on navigation it will reinitialize. I just tried PR and on going back from "I am new to status" screen animations starts from 0
- Screen with dark-overlay should not be animated #15534
- Even though other screens have dark-overlay and not animated, they should be at same progress as parent.
There was a problem hiding this comment.
Yep, a slight oversight in my initial commit. It should be addressed now. 👍
ilmotta
reviewed
Apr 19, 2023
| [quo2.core :as quo] | ||
| [quo2.foundations.colors :as colors] | ||
| [reagent.core :as reagent] | ||
|
|
| :index index | ||
| :header-text header-text | ||
| :header-background header-background | ||
| } (when content [content (* 4 window-width)])])] |
There was a problem hiding this comment.
Missing line break between } and (when and joining the } with the previous line.
| [view-id])) | ||
| (rn/use-effect | ||
| (fn [] | ||
| (when animate? |
There was a problem hiding this comment.
Just double-checking, but the effect wont' re-run if animate? changes, since it's not passed as a dependency to the vector. Is that what you intend?
There was a problem hiding this comment.
yes, previously it had
(when animate?
(rn/use-effect
(fn [] ... )))
but this looks dangerous to me as hooks should not be conditionally called. React will break if one render has a different number of hooks called then the previous render and so other issues like this.
So I thought this was a safer option
There was a problem hiding this comment.
hooks should not be conditionally called
100%
My comment was more related to the fact that if animate? changes, the hook won't rerun and it can be a source of a subtle bug. If animate? becomes false, the paused state won't be re-set. But I was just double-checking if it was your intention or not. Thanks!
There was a problem hiding this comment.
ok gotcha, yes in this case it was intentional. I wanted the hook to retrigger on view-id changing because it's supposed to pause/ unpause whenever it gets to the right page. I adjusted it slightly in my latest commit but it's similar
| :override-background-color colors/white-opa-5 | ||
| :style {:justify-self :flex-end | ||
| :margin-bottom 46 | ||
| :margin-horizontal 20}} (i18n/label :t/skip)]]]]))])) |
There was a problem hiding this comment.
Good catch, it's in the style guide https://guide.clojure.style/#vertically-align-fn-args.
| @@ -34,21 +31,21 @@ | |||
| paused))) | |||
|
|
|||
| (defn initialize-animation | |||
| [] | |||
| [progress paused] | |||
There was a problem hiding this comment.
Generally speaking I think what @J-Son89 did is the correct move. We should avoid global state outside components. It's a common discussion I've seen in many other PRs, perhaps a guideline should added.
|
@J-Son89, I don't know if you're aware, but if I press the hardware back button in this new Identifiers screen, it goes back to the "Generating keys" screen and stays there forever, broken. Then I have to close and open the app again, but at this point the account was already created and I can no longer go over the onboarding flow. I'm almost sure this is not a problem of your PR, but I think it's a serious UX bug nonetheless. |
hmm, I think it's possible to disable this in navigation. I'm so ios focused I forget about simple Android things like this :) I will check and see if I can include it |
jo-mut
approved these changes
Apr 20, 2023
|
@pavloburykh issues 2 & 3 should be address, however I'm curious if you can try issue 1 again. I added in some code to clean up the animations after they're used so hopefully if that was the issue then this would address it. I checked on an emulator and it seemed to have an improvement but will be interesting to see on a real device. |
J-Son89
force-pushed
the
jc/refactor-carousel
branch
from
81490c6
to
b8f9e26
Compare
90% of end-end tests have passedFailed tests (3)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Passed tests (26)Click to expandClass TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePR:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
|
Thanx @J-Son89! Issues 1 and 2 are fixed, but still reproducing ISSUE 3. Could you please take another look at it? Also, could you please clarify your comment regarding ring identifier:
|
Thanks @pavloburykh! Yes you are correct 1 - the identifier should be showing for a profile picture. This is a mistake related to this pr. 2 - the identifier should also be showing if the user does not set a profile picture, it should show their initials and their identifier however there is an issue as I described which we need to address to get the identifier correctly. I will create this issue but first needs to discuss how to implement it etc with some other devs. |
Comment on lines
30
to
31
| (defonce progress (reagent/atom nil)) | ||
| (defonce paused (reagent/atom nil)) |
There was a problem hiding this comment.
this is my clojurescript newbness :) should I just use a standard atom here? 🤔
There was a problem hiding this comment.
Updates to the shared value already get updated in view with the help of reanimated, so we don't need to take care of that. Also resetting the reagent atom will force update the view, so better to just use the standard atom here.
There was a problem hiding this comment.
thanks @Parveshdhull! Should I do the same on the Identifiers page?
Comment on lines
+43
to
+47
| (defn cleanup-animation | ||
| [progress paused] | ||
| (fn [] | ||
| (reanimated/cancel-animation @progress) | ||
| (reanimated/cancel-animation @paused))) |
There was a problem hiding this comment.
just curious, why we need this?
There was a problem hiding this comment.
QA found some performance issues on the Identifiers screens and afterwards when in the application (as a result of this pr). I figure we want to clear these animations after onboarding because they are not being used.
Adding this and refactoring the functional components to the new implementation seemed to address the issue so not 100% sure of which is needed.
Is there any reason we should not cleanup the animations after we are done with them?
There was a problem hiding this comment.
Is there any reason we should not cleanup the animations after we are done with them?
If it fixes the issue, there is no harm :)
Just a little curious, because as we have seen in the past once the view gets destroyed animation also stopped.
Example: #15522
J-Son89
force-pushed
the
jc/refactor-carousel
branch
from
b8f9e26
to
1502399
Compare
J-Son89
force-pushed
the
jc/refactor-carousel
branch
3 times, most recently
from
11b5061
to
78c5ca0
Compare
|
@pavloburykh the identity ring will now show if the profile picture is set 👍 |
|
also @pavloburykh I have created these follow up issues Ensure user avatar shows Identicon ring when no profile picture is set Adjust Identifiers page to do highlighting as shown in designs |
pavloburykh
force-pushed
the
jc/refactor-carousel
branch
from
78c5ca0
to
5fa570d
Compare
0% of end-end tests have passedNot executed tests (4)Failed tests (25)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
0% of end-end tests have passedNot executed tests (12)Failed tests (18)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
76% of end-end tests have passedNot executed tests (5)Failed tests (6)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Passed tests (19)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
|
@J-Son89 thanx for the PR. Tested and ready to merge. Failed e2e are not PR related. |
Thanks for all the work @pavloburykh! @Parveshdhull you're happy with how this pr looks? |
Parveshdhull
approved these changes
Apr 26, 2023
| :bottom 0 | ||
| :left 0 | ||
| :right 0 | ||
| }) |
There was a problem hiding this comment.
Please move these to above line
J-Son89
force-pushed
the
jc/refactor-carousel
branch
from
5fa570d
to
6a831ad
Compare
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: #15548
How it looks
To test: create a new account and go through the onboarding flows.
Please note, the designs have a feature which highlights each identifier as the respective description is shown on the carousel. This will be added in a follow up as pr was already quite large.
Additionally, I found a separate issue where the profile ring identifier is not working correctly, I will create a separate issue for this as it is a common problem across the application wherever we are using user_avatar.