Closed cammellos closed 4 months ago
is this enough to enable the new architecture on android
most likely just this. ref -> https://github.com/status-im/status-mobile/issues/18138#issuecomment-2066458833 We do not have build time failures, we just have this ugly pink thing which indicates incompatibility
I will grab this issue probably early next week due to other priorities. Until then, if someone wants to do it no problem 👍🏼
I will grab this issue probably early next week due to other priorities. Until then, if someone wants to do it no problem 👍🏼
Please whoever from mobile that's going to work on this too, assign it to yourself along with me :)
An update on the research of this issue:
If we just replace the blur-view
with a view
the app is not usable, since many views are just transparent:
https://github.com/status-im/status-mobile/assets/90291778/0e448969-5bef-4771-92c6-b378155d1e3f
Instead we can try finding the color being used and use an overlay with transparency.
Example, for shell we have neutral-80-opa-80
so if we change it for neutral-80
with opacity 92
(magic number I picked), it'd look as follows:
https://github.com/status-im/status-mobile/assets/90291778/8bce9561-6fa8-46fa-b038-7f96cfa7de08
We have 25 usages of the blur-view
component and 4 of the reanimated
's blur
, it'd be great if we find an abstraction to make this substitution easier, but even if not, I think it's doable.
Just use a solid color instead of finding a new opacity.
I tested this on a development build running on a Samsung Galaxy S23 Ultra, I felt the app is still slow :thinking: and I thought the improvement would be more significant. According to this issue the problems we have are:
Regarding to 1:
For sure the performance is improved, but IMO it's barely noticeable, maybe in some screens or other devices the improvement is more clear. It'd be great if we properly compare the performance before applying a solution.
And answering a question from this issue:
How much effort is to remove blur
Not too much in terms of refactoring, I'd say we need some days to properly refactor the code In the case we can't a color that looks good, we'll need to align with designers, so probably it'll delay it a bit more.
This is the code I used to test it (just a quick attempt) in react-native.blur
(def view* (reagent/adapt-react-class (.-BlurView blur)))
(defn view- []
(let [this (r/current-component)
props (r/props this)
children (r/children this)]
(if platform/android?
(into [rn/view props
[rn/view {:style {:position :absolute
:top 0
:left 0
:right 0
:bottom 0
:background-color (colors/alpha colors/neutral-80 0.92)
:z-index -10}}]]
children)
(into [view* props] children))))
(def vv (r/reactify-component view-))
(def view (r/adapt-react-class vv))
is this enough to enable the new architecture on android or there are more blockers
Sid is the expert on it, @siddarthkay could you please share with us the limitations you've found to enable it?
Thank you @ulisesmac. Excellent review of the solutions 👏🏼
To me, the main reason besides the ones you wrote is the cost of development for very little return to users, particularly for a product that still doesn't work well in so many areas. Even the fact that we're still discussing about blur in so many meetings and issues shows just how much wasted effort this is.
I surely like the blur implementation when we can nail it, but on Android I would vote for a simple solid replacement, the simplest and cheapest for now. I understand the concepts behind blur, but I believe users will be capable of understanding the context they are in and how things overlap without blur and without transparency (on Android). Removing blur probably won't affect revenue growth.
Unfortunately, as you correctly pointed out, we should now account for the cost of removing blur, which obviously not just pressing a magic button. I think if it's 2-3 days the cost of removal will pay off in the medium to long run. Just imagine that one more issue from a developer using blur can easily force that developer to waste hours or more and this cost is not easily measurable, but it's there.
the limitations you've found to enable it?
At the moment we can't get past the login so we will know once blur lib is removed from the project. All we know so far is that we do not have build issues which is nice, there may or may not be other runtime issues but we can't know for sure till we get past the login screen :)
@ilmotta
but on Android I would vote for a simple solid replacement, the simplest and cheapest for now.
Yeah, it's the easiest, we can take this approach.
@siddarthkay
At the moment we can't get past the login so we will know once blur lib is removed from the project.
"from the project" means that we also need to remove it from iOS? :open_mouth:
"from the project" means that we also need to remove it from iOS?
i think it will be removed only for Android, conditionally I guess
thank you @ulisesmac i would vote for the simple solid replacement, and It might not that visible improvement but we don't have like one big thing we can improve, overall app performance is the sum of small things like blur, so we have to improve them all
"from the project" means that we also need to remove it from iOS?
I assumed that would be easier for whoever is implementing the fallback.
But as long as BlurView
component is not rendered on Android thats all we need to move forward in enabling new architecture for react-native.
This can be achieved either by nuking the library OR by adding platform checks in the component we use for Blur View, I'd vote for whatever is easier/faster :)
This can be achieved either by nuking the library OR by adding platform checks in the component we use for Blur View, I'd vote for whatever is easier/faster :)
unfortunately, there are no options, we should keep it for iOS
@cammellos, since you opened this issue, do you think we have a satisfactory result and can close it? The consensus seems to be on using a solid color replacement. When do you think we should execute this decision to replace blur views? (the question goes to everyone else as well)
One of the last missing pieces is double-checking with designers the solid colors they would like to see been used on Android and if they can attach them in Figma to avoid the case of devs picking up colors by personal preference, but that can be done in a separate issue when we actually replace blur views. So not a blocker for us.
Ok, one last thing I want to add.
We can use React Native 0.74 (latest) with the blur library (https://github.com/Kureev/react-native-blur) and it compiles and works on Android, I'd say the same as before.
I confirmed it by updating a personal app that uses blur and that now it's running on RN 0.74, of course Status is a lot more complex, but I could use them without problems.
I know we have other arguments to remove the blur, and I'm not against it, I'm just sharing that maybe the blur is not a blocker for upgrading.
blur is not a blocker for upgrading.
Yeah we're not blocked for upgrading react-native, we're blocked for enabling new architecture in react-native.
We also have this in our react-native.config.js
file ->
https://github.com/status-im/status-mobile/blob/d2fb6c7ee08857dc988b3914a74479d6e265cf87/react-native.config.js#L20-L22
Which is probably why I saw this when trying to enable new architecture -> https://github.com/status-im/status-mobile/issues/18138#issuecomment-2066458833
With regards to upgrading react-native to 0.74 we discussed it in the offsite that we would rather not do it before July release because RN upgrades may cause some unknown side effects.
It is worth noting that last upgrade was smooth and didn't create many side effects, but some folks still feel the after effects of the ones before that :D
@ilmotta yes, thank you, I'll close this one and create an issue that is actionable
As discussed with designer, is fine to remove blur from android. Blur on android leads to some performance issue, complexity in the code and prevents us to enable to the new architecture for react native.
In this spike we should quantify the effort needed to remove blur, and what to do instead. @ulisesmac had some opinions on the matter.
After this spike, we should be able to answer roughly the questions:
1) how much effort is to remove blur 2) what are we going to do instead 3) is this enough to enable the new architecture on android or there are more blockers