chore: first pass at fixing collectibles transition #19945
Conversation
Jenkins BuildsClick to see older builds (44)
|
| :on-load-start #(set-load-time (fn [start-time] (- (datetime/now) start-time))) | ||
|
|
||
|
|
||
| :on-load-end (if (> load-time 200) |
There was a problem hiding this comment.
on-load-start and on-load-end can be moved to external functions. It has readability benefits and the potential to save renders.
There was a problem hiding this comment.
Yep for sure, I will add those types of details once I'm happy with the approach. This was just first pass 👌
|
The image transition is almost perfect. Made me feel that we somehow loaded colors before hand. The name and the logo seem to flicker in. A skeleton there will be nice too. |
Thanks, will add that! |
J-Son89
force-pushed
the
jc/collectible-transition
branch
from
23e7d7d
to
ebf6b31
Compare
| @@ -121,30 +170,43 @@ | |||
| (defn- image-view | |||
| [{:keys [avatar-image-src community? counter state set-state | |||
| gradient-color-index image-src supported-file?]}] | |||
| (let [theme (quo.theme/use-theme)] | |||
| (let [theme (quo.theme/use-theme) | |||
There was a problem hiding this comment.
I manually tested this for type image as well 👍
| @@ -105,23 +105,6 @@ | |||
| (def fast-create-community-enabled? | |||
| (enabled? (get-config :FAST_CREATE_COMMUNITY_ENABLED "0"))) | |||
|
|
|||
| (def default-multiaccount | |||
| on-collectible-long-press)) | ||
| :on-end-reached on-end-reached | ||
| :on-end-reached-threshold 4}]))) | ||
| ;; TODO: |
There was a problem hiding this comment.
@ulisesmac - what to do here, should we create a follow up issue? 🤔
There was a problem hiding this comment.
I think so, we can create a follow-up to improve the code and performance, it's not creating problems so we can address it when we have more time 👍
J-Son89
requested review from
rasom,
briansztamfater,
ajayesivan,
OmarBasem,
BalogunofAfrica,
ulisesmac and
shivekkhurana
| (reanimated/animate-shared-value-with-timing loader-opacity 0 timing-options-out :easing1) | ||
| (reanimated/animate-shared-value-with-timing image-opacity 1 timing-options-in :easing1) |
There was a problem hiding this comment.
Could be changed to
(reanimated/animate loader-opacity 0 timing-options-out)
(reanimated/animate image-opacity 1 timing-options-in)
| (if (> load-time 200) | ||
| (js/setTimeout | ||
| (fn [] | ||
| (set-state (fn [prev-state] | ||
| (assoc prev-state :image-loaded? true)))) | ||
| 500) | ||
| (set-state (fn [prev-state] | ||
| (assoc prev-state :image-loaded? true))))) | ||
|
|
||
| (defn on-load-error | ||
| [set-state] | ||
| (js/setTimeout (fn [] | ||
| (set-state (fn [prev-state] (assoc prev-state :image-error? true)))) | ||
| 800)) |
There was a problem hiding this comment.
Some magic numbers not defined (200, 500, 800)
| {:style (reanimated/apply-animations-to-style | ||
| {:opacity image-opacity} | ||
| (style/fallback {:theme theme}))} |
There was a problem hiding this comment.
Our guideline for apply-animations-to-style is to use it in the style file
Also we made a recent update to support passing an array of styles instead of using apply-animations-to-style https://github.com/status-im/status-mobile/blob/develop/doc/new-guidelines.md#pass-a-vector-of-styles-to-reanimated-view
There was a problem hiding this comment.
@OmarBasem - imo this guideline is most likely wrong and a bad idea.
As @ulisesmac pointed out recently, this function apply-animations-to-style is actually using a hook and should really be called use-animated-styles or something of this sort.
Hiding this in the style file is likely to lead to bugs (see @ajayesivan's comment below) and we should keep it in the view/ perhaps reconsider the format in how we pass these styled values.
There was a problem hiding this comment.
@J-Son89 I see, but are we still using the function apply-animations-to-style tho? What I currently see in the guidelines is we pass an array of styles:
{:style [{:opacity opacity
:transform [{:translate-x scroll-x}]}
{:flex-direction :row}]}
There was a problem hiding this comment.
aha, so you mean just remove the use of apply-animations-to-style completely?
That sounds better if so 👍
There was a problem hiding this comment.
@OmarBasem - imo this guideline is most likely wrong and a bad idea. As @ulisesmac pointed out recently, this function
apply-animations-to-styleis actually using a hook and should really be calleduse-animated-stylesor something of this sort. Hiding this in the style file is likely to lead to bugs (see @ajayesivan's comment below) and we should keep it in the view/ perhaps reconsider the format in how we pass these styled values.
@J-Son89 @OmarBasem @Parveshdhull Yeah, we need to update the guidelines and rename the animated functions to add the prefix use-
There was a problem hiding this comment.
aha, so you mean just remove the use of
apply-animations-to-stylecompletely? That sounds better if so 👍
@J-Son89 Be careful, actually the vector syntax doesn't behave exaclty the same in all cases. I've learnt by experience that it requires a default value in the regular styles section, otherwise sometimes the animation looks inconsistent at the beginning, this is only needed if your animation doesn't behave as expected:
Example, if we want to animate the opacity, you should provide the "initial" opacity in the regular styles:
[reanimated/view
{:style [;; Regular styles:
{,,,
:opacity 0 ;; This is the opacity at the beginning of the animation
,,,}
;; Animated styles:
{,,,
:opacity my-shared-value
,,,}
]}]There was a problem hiding this comment.
thanks @OmarBasem - yeah I think I noticed this alright. Strange, I'll update the uses here because likely it will affect. Thanks for the heads up 🙏
There was a problem hiding this comment.
added a default value to each style using opacity. all looks good now 👍
| {:style (reanimated/apply-animations-to-style | ||
| {:opacity loader-opacity} | ||
| (style/loading-image theme))} | ||
| [gradients/view |
There was a problem hiding this comment.
same here regarding reanimated/apply-animations-to-style
| [rn/view {:style style/card-details-container} | ||
| (cond (not (:avatar-loaded? state)) | ||
| (cond (and not community? (not (:avatar-loaded? state))) |
There was a problem hiding this comment.
Shouldn't this not be inside paranthesis () ?
There was a problem hiding this comment.
yes, will double check this. Thanks!
There was a problem hiding this comment.
addressed this, thanks for spotting!
| [rn/view {:style {:width 8}}] | ||
|
|
||
| [reanimated/view | ||
| {:style (reanimated/apply-animations-to-style |
There was a problem hiding this comment.
same here regarding reanimated/apply-animations-to-style
| collectible-name]]]) | ||
| collectible-name]] | ||
| :else [reanimated/view | ||
| {:style (reanimated/apply-animations-to-style |
There was a problem hiding this comment.
same here regarding reanimated/apply-animations-to-style
| loader-opacity (reanimated/use-shared-value (if supported-file? 1 0)) | ||
| image-opacity (reanimated/use-shared-value (if supported-file? 0 1)) | ||
| [load-time set-load-time] (rn/use-state (datetime/now)) | ||
| supported-file-styles (reanimated/apply-animations-to-style |
There was a problem hiding this comment.
Same reanimated/apply-animations-to-style
| loader-opacity (reanimated/use-shared-value (if supported-file? 1 0)) | ||
| image-opacity (reanimated/use-shared-value (if supported-file? 0 1)) | ||
| [load-time set-load-time] (rn/use-state (datetime/now)) | ||
| supported-file-styles (reanimated/apply-animations-to-style |
There was a problem hiding this comment.
Same reanimated/apply-animations-to-style
| (def collectible-container | ||
| {:padding 8 | ||
| :width "50%"}) |
There was a problem hiding this comment.
Could flex work here? (maybe :flex 0.5)
| [rn/view {:style {:width 8}}] | ||
|
|
||
| [reanimated/view | ||
| {:style (reanimated/apply-animations-to-style |
There was a problem hiding this comment.
Under the hood, reanimated/apply-animations-to-style uses a hook.
status-mobile/src/react_native/reanimated.cljs
Lines 120 to 123 in 3c4f72c
Here, we are calling it inside a condition, which is not ideal and can cause issues (see Rules of Hooks).
You are likely familiar with the hook rules, but the current function name doesn't indicate that it involves a hook.
I believe we decided to rename the function so that it starts with use-. cc: @Parveshdhull, @ulisesmac
There was a problem hiding this comment.
and can cause issues
Probably can throw error like #16140
There was a problem hiding this comment.
good point @ajayesivan. We should really update the naming for that method soon 👍
There was a problem hiding this comment.
Thank you @Parveshdhull
There was a problem hiding this comment.
Oh, I've seen @Parveshdhull already added the issue, Thank you!
J-Son89
force-pushed
the
jc/collectible-transition
branch
from
ebf6b31
to
941561f
Compare
| :number-of-lines 1 | ||
| :style style/card-detail-text} | ||
| collectible-name]]]) | ||
| (let [loader-opacity (reanimated/use-shared-value 1) |
There was a problem hiding this comment.
@OmarBasem - good spot on this section, was all a bit wrong but I have since refactored and fixed it. (I think it was my issue to begin with 😅 )
There was a problem hiding this comment.
No worries, thanks for fixing :)
|
use of apply-animations-to-style wasn't needed so I removed it in this pr 👍 |
J-Son89
force-pushed
the
jc/collectible-transition
branch
from
941561f
to
e8a9fcc
Compare
J-Son89
force-pushed
the
jc/collectible-transition
branch
from
788ffc8
to
d7f773d
Compare
| [{:opacity opacity} | ||
| {:opacity default-opacity-for-image |
There was a problem hiding this comment.
👍 nice you added these default values
| (defn loading-image-with-opacity | ||
| [theme opacity] | ||
| [{:opacity opacity} | ||
| (loading-image theme)]) | ||
|
|
||
| (defn avatar-container | ||
| [loaded?] | ||
| {:flex 1 | ||
| :flex-direction :row | ||
| :opacity (when-not loaded? 0)}) | ||
| [opacity] | ||
| [{:opacity opacity} | ||
| {:flex 1 | ||
| :flex-direction :row | ||
| :opacity default-opacity-for-image}]) | ||
|
|
||
| (defn supported-file | ||
| [opacity] | ||
| [{:opacity opacity} | ||
| {:aspect-ratio 1 | ||
| :opacity default-opacity-for-image}]) |
There was a problem hiding this comment.
Good to see the vector syntax is being used 👍 it's great because when it's possible to be used, it removes the complexities of dealing with a hook 👍
| :number-of-lines 1 | ||
| :style style/card-detail-text} | ||
| collectible-name]] | ||
| (when (not (:avatar-loaded? state)) |
| (defn- on-collectible-press | ||
| [{:keys [id]}] | ||
| (rf/dispatch [:wallet/get-collectible-details id])) | ||
|
|
||
| (defn- on-collectible-long-press | ||
| [{:keys [preview-url collectible-details]}] | ||
| (rf/dispatch [:show-bottom-sheet | ||
| {:content (fn [] | ||
| [options-drawer/view | ||
| {:name (:name collectible-details) | ||
| :image (:uri preview-url)}])}])) | ||
|
|
||
| (defn- on-end-reached | ||
| [] | ||
| (rf/dispatch [:wallet/request-collectibles-for-current-viewing-account])) | ||
|
|
There was a problem hiding this comment.
BTW, this was part of the solution to avoid the re-renders, previously they weren't stable references, so they caused a re-render on every collectible
87% of end-end tests have passedFailed tests (4)Click to expandClass TestWalletMultipleDevice:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Expected to fail tests (3)Click to expandClass TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (45)Click to expandClass TestDeepLinksOneDevice:
Class TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePR:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestWalletOneDevice:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
mariia-skrypnyk
force-pushed
the
jc/collectible-transition
branch
from
5cd2106
to
f577e9f
Compare
|
Hey @J-Son89 ! Thanks for the PR. |
|
Thanks @mariia-skrypnyk and thanks @ulisesmac for the help on this pr 💪🏼 |
Fix hidden hook usage Improve logging for unsupported collectibles Fix re-rendering of collectibles in flat-list chore: clean up implementation and speed up animation remove apply animations to style chore: use default opacity chore: use default opacity chore: add issue to todo chore: use flex chore: flex Avatars/Community Avatar Component (#20147) Avatars/dApp Avatar Component (#20145) Update eth-archival pokt url Remove not implemented Notification settings from community longtap m… (#20169) pick between JSC & Hermes for Android (#20171) We implement both `JSC` and `Hermes` in build phase of `Android` which increases our `APK` size by ~ `2 MB`. This was fine before but currently we have to get below the `100 MB` limit. This commit implements the preferred engine after inferring `hermesEnabled` property from gradle.properties This property is modified at build time for release here https://github.com/status-im/status-mobile/blob/178d62bd276afffef5fe7a3f773e390d83336d9c/nix/mobile/android/build.nix#L17 and set for debug here https://github.com/status-im/status-mobile/blob/178d62bd276afffef5fe7a3f773e390d83336d9c/Makefile#L280 Which should further reduce the `APK` size by `2 MB`. [#19232] - Fix derivation path generation and keypair creation (#19531) * Add more default dependencies to slide button * Fix wallet account creation: derivation paths and keypairs
J-Son89
force-pushed
the
jc/collectible-transition
branch
from
5f288f6
to
bde0a3b
Compare
fixes: #19678
This pr smoothes the transition for the collectibles cards loading to a loaded image.
On implementing I realised there is an issue when the image is already loaded as the image is cached and will load quite quickly anyway. To handle this I am measuring the load time. This works fine if the user only has a small number of images.
With help from @ulisesmac we also discovered a bug where the collectibles were re rendering a lot on scroll. This is also addressed in this pr and the overall scrolling experience is a lot smoother now.
Latest reference video:
Screen.Recording.2024-05-20.at.15.07.22.mov
Note: The animations being slower looks better for one load, but if the user goes back and forth from the tabs several times then it just becomes a bit irritating after a while.