-
Notifications
You must be signed in to change notification settings - Fork 985
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
[#21658] - Avatars blinking #21782
base: develop
Are you sure you want to change the base?
[#21658] - Avatars blinking #21782
Changes from all commits
f6703cd
85c0a9b
604f5a1
6a7dc38
e079b83
0bd6156
0560f79
99b30d2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,40 +1,70 @@ | ||
(ns react-native.fast-image | ||
(:require | ||
["react-native-fast-image" :as FastImage] | ||
[clojure.string :as string] | ||
[oops.core :as oops] | ||
[react-native.core :as rn] | ||
[reagent.core :as reagent])) | ||
|
||
(def fast-image-class (reagent/adapt-react-class ^js FastImage)) | ||
(defn- build-source | ||
[source] | ||
(if (string? source) | ||
{:uri source | ||
:priority :high} | ||
source)) | ||
|
||
(defn placeholder | ||
[style child] | ||
[rn/view {:style (merge style {:flex 1 :justify-content :center :align-items :center})} | ||
child]) | ||
(defn- remove-port | ||
[source] | ||
(if (string? source) | ||
(string/replace-first source #":\d+" "") | ||
(string/replace-first (oops/oget source :uri) #":\d+" ""))) | ||
|
||
(defn fast-image | ||
(defn- placeholder | ||
[{:keys [style fallback-content error? loaded?]}] | ||
[rn/view | ||
{:style (assoc style | ||
:flex 1 | ||
:justify-content :center | ||
:align-items :center)} | ||
(cond | ||
(and error? fallback-content) fallback-content | ||
error? [rn/text "X"] | ||
(not loaded?) [rn/activity-indicator {:animating true}])]) | ||
|
||
;; We cannot use hooks since `reactify-component` seems to ignore the functional compiler | ||
(defn- internal-fast-image | ||
[_] | ||
(let [loaded? (reagent/atom false) | ||
error? (reagent/atom false)] | ||
(fn [{:keys [source fallback-content] :as props}] | ||
[fast-image-class | ||
(merge | ||
props | ||
{:source (if (string? source) | ||
{:uri source | ||
:priority :high} | ||
source) | ||
:on-error (fn [e] | ||
(when-let [on-error (:on-error props)] | ||
(on-error e)) | ||
(reset! error? true)) | ||
:on-load (fn [e] | ||
(when-let [on-load (:on-load props)] | ||
(on-load e)) | ||
(reset! loaded? true) | ||
(reset! error? false))}) | ||
(let [loaded? (reagent/atom false) | ||
error? (reagent/atom false) | ||
on-image-error (fn [event on-error] | ||
(when (fn? on-error) (on-error event)) | ||
(reset! error? true)) | ||
on-image-loaded (fn [event on-load] | ||
(when (fn? on-load) (on-load event)) | ||
(reset! loaded? true) | ||
(reset! error? false))] | ||
(fn [{:keys [source fallback-content on-error on-load] :as props}] | ||
[:> FastImage | ||
(assoc props | ||
:source (build-source source) | ||
:on-error #(on-image-error % on-error) | ||
:on-load #(on-image-loaded % on-load)) | ||
(when (or @error? (not @loaded?)) | ||
[placeholder (:style props) | ||
(if @error? | ||
(or fallback-content [rn/text "X"]) | ||
(when-not @loaded? | ||
[rn/activity-indicator {:animating true}]))])]))) | ||
[placeholder | ||
{:style (js->clj (:style props)) | ||
:fallback-content fallback-content | ||
:error? @error? | ||
:loaded? @loaded?}])]))) | ||
|
||
(defn- compare-props | ||
[old-props new-props] | ||
(let [old-source (some-> old-props :source remove-port) | ||
new-source (some-> new-props :source remove-port)] | ||
(and (= old-source new-source) | ||
(= (dissoc old-props :source) (dissoc new-props :source))))) | ||
|
||
(def fast-image | ||
(-> internal-fast-image | ||
(reagent/reactify-component) | ||
(rn/memo compare-props) | ||
(reagent/adapt-react-class))) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,19 +23,22 @@ | |
(when f | ||
(f data index)))) | ||
|
||
(defn dissoc-custom-props | ||
[props] | ||
(dissoc props :data :header :footer :empty-component :separator :render-fn :key-fn :on-drag-end-fn)) | ||
|
||
(defn base-list-props | ||
[{:keys [key-fn render-fn empty-component header footer separator data render-data on-drag-end-fn] | ||
[{:keys [key-fn data render-fn empty-component header footer separator render-data on-drag-end-fn] | ||
:as props}] | ||
(merge | ||
{:data (to-array data)} | ||
(when key-fn {:keyExtractor (wrap-key-fn key-fn)}) | ||
(when render-fn {:renderItem (wrap-render-fn render-fn render-data)}) | ||
(when separator {:ItemSeparatorComponent (fn [] (reagent/as-element separator))}) | ||
(when empty-component {:ListEmptyComponent (fn [] (reagent/as-element empty-component))}) | ||
(when header {:ListHeaderComponent (reagent/as-element header)}) | ||
(when footer {:ListFooterComponent (reagent/as-element footer)}) | ||
(when on-drag-end-fn {:onDragEnd (wrap-on-drag-end-fn on-drag-end-fn)}) | ||
(dissoc props :data :header :footer :empty-component :separator :render-fn :key-fn :on-drag-end-fn))) | ||
(cond-> {:data (to-array data)} | ||
key-fn (assoc :keyExtractor (wrap-key-fn key-fn)) | ||
render-fn (assoc :renderItem (wrap-render-fn render-fn render-data)) | ||
separator (assoc :ItemSeparatorComponent (fn [] (reagent/as-element separator))) | ||
empty-component (assoc :ListEmptyComponent (fn [] (reagent/as-element empty-component))) | ||
header (assoc :ListHeaderComponent (reagent/as-element header)) | ||
footer (assoc :ListFooterComponent (reagent/as-element footer)) | ||
on-drag-end-fn (assoc :onDragEnd (wrap-on-drag-end-fn on-drag-end-fn)) | ||
:always (merge (dissoc-custom-props props)))) | ||
|
||
Comment on lines
30
to
42
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. multiple |
||
(defn flat-list | ||
[props] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,10 @@ | ||
(ns status-im.common.scalable-avatar.style) | ||
|
||
(defn wrapper | ||
[{:keys [scale margin-top margin border-color]}] | ||
[{:transform [{:scale scale}] | ||
:margin-top margin-top | ||
:margin-left margin | ||
:margin-bottom margin} | ||
{:border-width 4 | ||
:border-color border-color | ||
:border-radius 100}]) | ||
[border-color scale] | ||
[{:transform-origin "bottom left" | ||
:border-width 4 | ||
:border-color border-color | ||
:border-radius 100 | ||
:transform [{:scale 1} {:translate-y 4}]} | ||
{:transform [{:scale scale} {:translate-y 4}]}]) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,30 +2,14 @@ | |
(:require [quo.core :as quo] | ||
[react-native.reanimated :as reanimated] | ||
[status-im.common.scalable-avatar.style :as style])) | ||
(def scroll-animation-input-range [0 50]) | ||
(def header-extrapolation-option | ||
{:extrapolateLeft "clamp" | ||
:extrapolateRight "clamp"}) | ||
|
||
(defn f-avatar | ||
(def scroll-range #js [0 48]) | ||
(def scale-range #js [1 0.4]) | ||
|
||
(defn view | ||
[{:keys [scroll-y full-name online? profile-picture customization-color border-color]}] | ||
(let [image-scale-animation (reanimated/interpolate scroll-y | ||
scroll-animation-input-range | ||
[1 0.4] | ||
header-extrapolation-option) | ||
image-top-margin-animation (reanimated/interpolate scroll-y | ||
scroll-animation-input-range | ||
[0 20] | ||
header-extrapolation-option) | ||
image-side-margin-animation (reanimated/interpolate scroll-y | ||
scroll-animation-input-range | ||
[-4 -20] | ||
header-extrapolation-option)] | ||
[reanimated/view | ||
{:style (style/wrapper {:scale image-scale-animation | ||
:margin-top image-top-margin-animation | ||
:margin image-side-margin-animation | ||
:border-color border-color})} | ||
(let [image-scale (reanimated/interpolate scroll-y scroll-range scale-range :clamp)] | ||
[reanimated/view {:style (style/wrapper border-color image-scale)} | ||
Comment on lines
-10
to
+12
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Avatar animation fix |
||
[quo/user-avatar | ||
{:full-name full-name | ||
:online? online? | ||
|
@@ -34,7 +18,3 @@ | |
:ring? true | ||
:customization-color customization-color | ||
:size :big}]])) | ||
|
||
(defn view | ||
[props] | ||
[:f> f-avatar props]) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -62,8 +62,8 @@ | |
:on-end-reached #(re-frame/dispatch [:chat/show-more-chats]) | ||
:keyboard-should-persist-taps :always | ||
:data items | ||
:render-fn (fn [item] | ||
(chat-list-item/chat-list-item item theme)) | ||
:render-data {:theme theme} | ||
:render-fn chat-list-item/chat-list-item | ||
Comment on lines
-65
to
+66
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix rerenders on items for this flat-list |
||
:scroll-event-throttle 8 | ||
:content-container-style {:padding-bottom | ||
shell.constants/floating-shell-button-height | ||
|
@@ -94,8 +94,7 @@ | |
{:selected-tab :tab/contacts | ||
:tab->content (empty-state-content theme)}] | ||
[rn/section-list | ||
{:ref (when (not-empty items) | ||
set-scroll-ref) | ||
{:ref (when (seq items) set-scroll-ref) | ||
:key-fn :public-key | ||
:get-item-layout get-item-layout | ||
:content-inset-adjustment-behavior :never | ||
|
@@ -108,8 +107,8 @@ | |
:sticky-section-headers-enabled false | ||
:render-section-header-fn contact-list/contacts-section-header | ||
:render-section-footer-fn contact-list/contacts-section-footer | ||
:render-fn (fn [data] | ||
(contact-item-render data theme)) | ||
:render-data {:theme theme} | ||
:render-fn contact-item-render | ||
:scroll-event-throttle 8 | ||
:on-scroll #(common.banner/set-scroll-shared-value | ||
{:scroll-input (oops/oget % "nativeEvent.contentOffset.y") | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,11 @@ | ||
(ns status-im.contexts.profile.settings.header.style | ||
(:require [quo.foundations.colors :as colors])) | ||
(:require [quo.foundations.colors :as colors] | ||
[react-native.platform :as platform])) | ||
|
||
(def avatar-row-wrapper | ||
{:display :flex | ||
:padding-left 20 | ||
{:padding-left 20 | ||
:padding-right 12 | ||
:margin-top -60 | ||
:margin-bottom -4 | ||
:margin-top -65 | ||
:align-items :flex-end | ||
:justify-content :space-between | ||
:flex-direction :row}) | ||
|
@@ -23,13 +22,8 @@ | |
:flex-direction :row | ||
:justify-content :space-between}) | ||
|
||
(defn avatar-container | ||
[theme scale-animation top-margin-animation side-margin-animation] | ||
[{:transform [{:scale scale-animation}] | ||
:margin-top top-margin-animation | ||
:margin-left side-margin-animation | ||
:margin-bottom side-margin-animation} | ||
{:align-items :flex-start | ||
:border-width 4 | ||
:border-color (colors/theme-colors colors/border-avatar-light colors/neutral-80-opa-80 theme) | ||
:border-radius 100}]) | ||
(defn avatar-border-color | ||
[theme] | ||
(if platform/android? | ||
colors/neutral-80-opa-80 ;; Fix is not needed because Android doesn't use blur | ||
(colors/theme-colors colors/border-avatar-light colors/neutral-80-opa-80 theme))) | ||
Comment on lines
+25
to
+29
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Avatar's ring fix There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Worth noting that we have an issue that we can play in 2.33 to remove identity rings #21743 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This ring I'm referring to is the one showed in PR's video: avatar-circle-not-matching.mp4different to the identity ring |
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'm not sure, but does
oops/get
throw an exception if it doesn't find:uri
?If so, it might be worth using
goog.object/get
with a default value like:(goog.object/get source :uri "")
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 question @seanstrom. Some time ago I changed the default behavior of oops during development, which is to instrument calls and throw on missing keys. Now it just warns. In advanced compilation instrumentation in oops is disabled by default and we don't have to configure anything special.
status-mobile/src/status_im/setup/oops.cljs
Lines 5 to 12 in eae887e