Remove 100ms delay when opening image #15422
Conversation
Jenkins BuildsClick to see older builds (12)
|
OmarBasem
force-pushed
the
remove-image-delay
branch
from
ecba5ad to
c17b0ce
Compare
| {:events [:chat.ui/navigate-to-lightbox]} | ||
| [{:keys [db]} shared-element-id screen-params] | ||
| (let [updated-db (assoc db :shared-element-id shared-element-id)] | ||
| (reagent/next-tick #(rf/dispatch [:navigate-to :lightbox screen-params])) |
There was a problem hiding this comment.
Is it safe to use reagent/next-tick inside a re-frame event?
And if we need it to happen immediately .. Why no use dispatch-sync?
Should we go with re-frame.interop/next-tick
CC: @flexsurfer, @ilmotta
There was a problem hiding this comment.
And if we need it to happen immediately .. Why no use dispatch-sync?
@ibrkhalil we do not want the navigate-to to happen immediately. (shared-element-id should dispatch first)
Should we go with day8/re-frame#480 (comment)
I just tried this one and it did not work. It navigated without executing the shared element transition. (same for dispatch-later)
There was a problem hiding this comment.
So I guess a normal :dispatch key in the returned map didn't work too .. Cool cool
Thanks Omar
There was a problem hiding this comment.
shared-element-id should dispatch first
could you elaborate what does it mean?
There was a problem hiding this comment.
shared-element-id is used to execute the shared element transition animation. When pressing on an image, we need to update the shared-element-id with that image's id before navigating in order for the animation to execute.
Note that the shared-element-transition for the opening animation currently does not work on Android.
There was a problem hiding this comment.
@ibrkhalil, I don't know the answer to your question. I've never done this workaround with next-tick inside the event handler. It's dirty, but if it's the only thing @OmarBasem found to work for now.
Something tells me if we used :db and :fx keywords like re-frame suggests, we would be able to correctly order the effects to happen after the :db. Unfortunately we can't, unless we change/improve the rf/defn macro.
Just for reference:
In fact, with v1.1.0 best practice changed to event handlers should only return two effects :db and :fx, in which case :db was always done first and then :fx, and within :fx the ordering is sequential. This new approach is more about making it easier to compose event handlers from many smaller functions, but more specificity around ordering was a consequence.
There was a problem hiding this comment.
@ilmotta I am curious to know what is the difference between what we are currently doing and what you are suggesting here
There was a problem hiding this comment.
The way we dispatch effects in re-frame is kind of deprecated. But in order to use :db and :fx we need to change the rf/defn macro. I'm curious too @OmarBasem, because I'm not sure if it would solve your problem. You can totally try this if you want, you'll just need to use re-frame.core/reg-event-fx instead of rf/defn, just don't expect the code to be approved since we currently have a guideline to always use rf/defn.
| (rf/defn navigate-to-lightbox | ||
| {:events [:chat.ui/navigate-to-lightbox]} | ||
| [{:keys [db]} shared-element-id screen-params] | ||
| (let [updated-db (assoc db :shared-element-id shared-element-id)] |
There was a problem hiding this comment.
why updated-db is needed ?
OmarBasem
force-pushed
the
remove-image-delay
branch
from
c17b0ce to
1e6c4e1
Compare
94% of end-end tests have passedFailed tests (2)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestDeeplinkOneDeviceNewUI:
Passed tests (31)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestDeeplinkOneDeviceNewUI:
Class TestGroupChatMultipleDeviceMergedNewUI:
|
94% of end-end tests have passedFailed tests (2)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestDeeplinkOneDeviceNewUI:
Passed tests (31)Click to expandClass TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestDeeplinkOneDeviceNewUI:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterContactRequestMultipleDevicePR:
|
|
hi @OmarBasem thank you for your work. No issues from my side. PR is ready to be merged |
OmarBasem
force-pushed
the
remove-image-delay
branch
from
1e6c4e1 to
614b18a
Compare
fixes: #15358
QA notes:
What needs to be tested is that when pressing on an image, the opening animation executes. Currently, this works only on iOS. For Android there is an open issue: #15421
Also note that there is some open issues for images (#15395, #15391)