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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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! |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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.
unused 👍
on-collectible-long-press)) | ||
:on-end-reached on-end-reached | ||
:on-end-reached-threshold 4}]))) | ||
;; TODO: |
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.
@ulisesmac - what to do here, should we create a follow up issue? 🤔
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.
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 👍
(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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be changed to
(reanimated/animate loader-opacity 0 timing-options-out)
(reanimated/animate image-opacity 1 timing-options-in)
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.
sounds good 👍
(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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 calleduse-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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aha, so you mean just remove the use of
apply-animations-to-style
completely? 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this not
be inside paranthesis ()
?
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.
yes, will double check this. Thanks!
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same reanimated/apply-animations-to-style
(def collectible-container | ||
{:padding 8 | ||
:width "50%"}) |
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.
Could flex
work here? (maybe :flex 0.5
)
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.
will try. 👍
[rn/view {:style {:width 8}}] | ||
|
||
[reanimated/view | ||
{:style (reanimated/apply-animations-to-style |
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.
Under the hood, reanimated/apply-animations-to-style
uses a hook.
status-mobile/src/react_native/reanimated.cljs
Lines 120 to 123 in 3c4f72c
(defn apply-animations-to-style | |
[animations style] | |
(use-animated-style | |
(worklets.core/apply-animations-to-style (clj->js animations) (clj->js style)))) |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and can cause issues
Probably can throw error like #16140
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.
good point @ajayesivan. We should really update the naming for that method soon 👍
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.
logged an issue #20110
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.
Thank you @Parveshdhull
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.
Oh, I've seen @Parveshdhull already added the issue, Thank you!
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries, thanks for fixing :)
use of apply-animations-to-style wasn't needed so I removed it in this pr 👍 |
941561f
to
e8a9fcc
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.
Thanks for refactoring reanimated styles. LGTM!
788ffc8
to
d7f773d
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.
I like how they look on the video 💯
the code looks good too 👍
[{:opacity opacity} | ||
{:opacity default-opacity-for-image |
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.
👍 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)) |
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.
when-not
👀
(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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 passed
Failed 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:
|
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.
Nice work @J-Son89 🎉
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
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.