-
Notifications
You must be signed in to change notification settings - Fork 984
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
Lightbox refactoring #16096
Lightbox refactoring #16096
Conversation
Jenkins BuildsClick to see older builds (12)
|
d420bb1
to
df03b6f
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.
It's great to see this PR @OmarBasem, thanks for trying to improve the timers cancellation problem.
I understand it's hard to solve the problem without the timers
atom, given how the code is written, so I don't have anything super useful to suggest here.
I'm giving my approval, but before merging, do consider renaming the the keys inside the timers
atom map. show-0
, mount-1
, etc are not conveying any meaning.
@@ -15,24 +15,37 @@ | |||
[status-im2.contexts.chat.lightbox.constants :as constants] | |||
[utils.worklets.lightbox :as worklet])) | |||
|
|||
(defn clear-timers | |||
[timers] | |||
(js/clearTimeout (:mount-0 @timers)) |
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 could give more specific names, like what is :mount-0
referring to?
82% of end-end tests have passed
Failed tests (6)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Passed tests (27)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
lint toggle opacity updates updates updates updates updates updates updates
This PR continues lightbox refactoring.
It moves the state initialization in
zoomable-image
component outside of the renderer, and clears timers on component unmount.Adding for context, there was no problem with having the state initialization outside of the renderer. The problem is in RNN Shared Element Transition. Opened an issue in there: wix/react-native-navigation#7726