Implement onboarding slide animation #15489
Conversation
Jenkins BuildsClick to see older builds (36)
|
Parveshdhull
force-pushed
the
feat/improve-onboarding-screen
branch
6 times, most recently
from
71c4fed to
e91e858
Compare
Parveshdhull
changed the title
Parveshdhull
requested review from
ulisesmac,
J-Son89,
briansztamfater and
flexsurfer
|
@Parveshdhull, I see you deleted the single versions of these images. I think we still need these as they are the backround for the pages that follow on the onboarding process. |
| (when dark-overlay? | ||
| [:f> | ||
| (fn [] | ||
| (carousel.animation/initialize-animation) |
There was a problem hiding this comment.
why is the carousel here? 🤔
nearly all of the onboarding pages are using this background component except for the intro page and the intro page is using the carousel component.
Should we not create a separate background component for this use case?
There was a problem hiding this comment.
nearly all of the onboarding pages are using this background component except for the intro page and the intro page is using the carousel component.
Hi, I don't understand what you mean, intro page is also using background component
There was a problem hiding this comment.
ah you are right , it was not before. I understand now the other pages are using this for where the background image has stopped and the section being shown. Nicely done :D
There was a problem hiding this comment.
I'd use useEffect hook with empty array to do initialization stuff, any reason to not do it?
There was a problem hiding this comment.
Hi @briansztamfater,
According to the react docs - Effects let you specify side effects that are caused by rendering itself
So, if I use effect here then the animation will be initialized after rendering.
But I need to initialize it before rendering, otherwise, shared-values will be nil and will throw an error.
There was a problem hiding this comment.
what about useLayoutEffect? 🤔 https://react.dev/reference/react/useLayoutEffect
There was a problem hiding this comment.
Just curious, how this will be different then calling function before view?
There was a problem hiding this comment.
I'm not 100% if this applies here, but useLayoutEffect works similar to useEffect except it fires before the screen is painted. It's probably safer to initialise this way in React if possible. But it's not a must have, just worth exploring :)
Hi @J-Son89, Thank you very much for reviewing PR. |
| case (progressValue < 25): | ||
| return 0; | ||
| break; | ||
| case (progressValue == 25): | ||
| return withTiming(-windowWidth, easeOut); | ||
| break; | ||
| case (progressValue < 50): | ||
| return -windowWidth; | ||
| break; | ||
| case (progressValue == 50): | ||
| return withTiming(-2 * windowWidth, easeOut); | ||
| break; | ||
| case (progressValue < 75): | ||
| return -2 * windowWidth; | ||
| break; | ||
| case (progressValue == 75): | ||
| return withTiming(-3 * windowWidth, easeOut); | ||
| break; | ||
| case (progressValue < 100): | ||
| return -3 * windowWidth; | ||
| break; | ||
| case (progressValue == 100): | ||
| return withTiming(-4 * windowWidth, easeOut); | ||
| break; | ||
| default: | ||
| return 0; | ||
| break; |
There was a problem hiding this comment.
Maybe improve indentation
switch(expression) {
case x:
// code block
break;
case y:
// code block
break;
default:
// code block
}
There was a problem hiding this comment.
Also break statements are not necessary as you are returning right before breaks
There was a problem hiding this comment.
Maybe improve indentation
Can we do code formatting also for js files? cc @yqrashawn
src/status_im2/contexts/onboarding/common/carousel/animation.cljs
Outdated
Show resolved
Hide resolved
| [react-native.reanimated :as reanimated] | ||
| [utils.worklets.onboarding-carousel :as worklets.onboarding-carousel])) | ||
|
|
||
| (def progress (atom nil)) |
There was a problem hiding this comment.
I wonder is it possible to avoid these global atoms and instead have them scoped to the onboarding stack? 🤔
There was a problem hiding this comment.
Maybe, we can initialize them on top of root and pass as params to other stacks.
Although we have two roots, we have to do this initialization in profiles & intro.
But just curious why we want to do that?
There was a problem hiding this comment.
we can initialize them on top of root and pass as params to other stacks
It sounds complicated to pass shared values as params to navigated screens
There was a problem hiding this comment.
Yeah I believe we want to avoid global atoms as much as possible
I understand there are exceptions and perhaps it's needed/ not so much of an issue in this case
cc: @ilmotta @ulisesmac
There was a problem hiding this comment.
Thanks for cc'ing me @J-Son89. Let's be real, global state is often not a big issue since most of our global atoms or ratoms are used only in the scope of a single namespace, at least that's how I feel. Practically speaking, it's passable 🤷♂️
But having said that, I still think it's weird... In one hand Clojure(Script) promotes pure code as much as possible and controlled, local state. On the other hand, ClojureScript and Reagent allow us to define state used for views outside view components, something that's a no-no in React itself. That is to say, even raw React code feels more functional than our own ClojureScript code with global vars.
It's a discussion without a clear answer to me. As usual, almost all global state is created in the name of convenience, so this is not enough to justify, at least to me.
Given this discussion come and go every now and then, maybe it's about time we write something in the guidelines if we can all agree on something. Perhaps the discussion should be made outside this PR?
There was a problem hiding this comment.
yeah I think it would be nice to discuss this. As far as I see it one big drawback is this code becomes a lot less reusable. In this case we have other carousels in the new designs and so ideally we could use this component/code (or at least a good chunk of it) to create these carousels. Anyway if that is the case we can also let the developer who will work on these aim to refactor when they go to use this code. Ideally we can do as much as possible to enable that from the initial point code is added to the code base.
There was a problem hiding this comment.
Great point, reusability is one big reason to favor local state, because there's no state to synchronize between components in the first place.
Parveshdhull
force-pushed
the
feat/improve-onboarding-screen
branch
from
82163cb to
078f22e
Compare
97% of end-end tests have passedFailed tests (1)Click to expandClass TestCommunityMultipleDeviceMerged:
Passed tests (28)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
|
Hi, @Parveshdhull thank you for PR. One low-priority issue is found: ISSUE 1: [Androids only] The animation is paused if the Status app is closed by device's "back" buttonSteps to reproduce:
Actual result:The animation is paused stops.mp4Expected result:The animation still sliding after app reopening ENV:
|
|
hi @VladimrLitvinenko, |
@Parveshdhull Sure. No other issues from my side. PR is ready to be merged. Created this issue as a separate follow up #15522 |
Parveshdhull
force-pushed
the
feat/improve-onboarding-screen
branch
from
078f22e to
4805234
Compare
| :end {:x 0 :y 1} | ||
| :style (style/background-gradient-overlay dark-overlay?)}] | ||
| (when dark-overlay? | ||
| [:f> |
There was a problem hiding this comment.
I'd recommend to put that functional component in a different function:
(defn my-f-component [props]
;;component code
)
(defn component-exposed-to-the-world [props]
[:f> my-f-component props])
fixes: #15012
fixes: #15270
fixes: #15274
fixes: #15493
Summary
PR Implements onboarding slide animation
Note: Changing the slide by swiping left, or right will be implemented in a separate PR
status: ready