-
Notifications
You must be signed in to change notification settings - Fork 987
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
[Spike] Investigate removing blur from android in order to enable the new architecture #20066
Comments
most likely just this. |
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: Approach 1 (not good)If we just replace the Screen_Recording_20240521_145858_Status.Debug.mp4Approach 2Instead we can try finding the color being used and use an overlay with transparency. Screen_Recording_20240521_150017_Status.Debug.mp4We have 25 usages of the Approach 3Just use a solid color instead of finding a new opacity. Important notesI tested this on a development build running on a Samsung Galaxy S23 Ultra, I felt the app is still slow 🤔 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:
Not too much in terms of refactoring, I'd say we need some days to properly refactor the code CodeThis is the code I used to test it (just a quick attempt) in (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)) |
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. |
At the moment we can't get past the login so we will know once blur lib is removed from the project. |
Yeah, it's the easiest, we can take this approach.
"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 |
I assumed that would be easier for whoever is implementing the fallback. But as long as 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. |
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 status-mobile/react-native.config.js Lines 20 to 22 in d2fb6c7
Which is probably why I saw this when trying to enable new architecture -> #18138 (comment) 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:
The text was updated successfully, but these errors were encountered: