fix: switch to RN native ActivityIndicator#2000
Conversation
react-native-indicators spinner is unmaintained and incompatible with the new architecture (it was causing rendering issues and running CPU to 100%) Note: this is a visual change: the spinner is now the android platform-native material design spinner
|
08b6d3e to
04d3a3f
Compare
|
Do you think design should weigh on in the new look? screen-20260716-044238-1784212948699.mp4 |
Yes, good idea, can you consult? Is the behavior different? I think color and size we can tweak immediately. The work of fixing the old spinner style, or making our own spinner to match the style of the old one, is too much work short-term unless we feel really strongly about having our own spinner. |
I asked and am waiting on a response as of July 21. |
…an still be overridden.
There was a problem hiding this comment.
@ErikSin or @gmaclennan do you want to look at the changes I made? I set the default color to comapeo-blue as Aldo suggested, fixed some missing usages of the new Loading Indicator and merged with develop. Earlier I had gone through and looked at all of the places it is used and they all seemed to do the trick as far as I could tell. I just feel a little weird approving code that I wrote? But it is only a bit at the end.
|
@cimigree Do you think it is a good idea to maintain a number size for android, and then a different prop for iOS? If so, then i think we need to create the component that automatically maintains that, so we don't have to do that for each instance of it. IMO i think we should try to use |
I agree. I think small and large look fine to me and if and until design has a strong opinion otherwise, than I would rather just have one look/ one set of code for ios and android. There will be enough places where we need to have different code, so here this seems to work to share it. |
Oh I see that is already happening. Were passing a size, but then forcing that into a "small" and "large". I didn't fully understand the code. I think we should just get rid of that and just have the prop "Large" or "small", and make that part of this refactor. It will just make it more consistent, and less confusing instead of using a number that is then turned into an enum |
|
ErikSin
left a comment
There was a problem hiding this comment.
I have one blocking comment about the e2e test animations. It would simplify the code in many place, and make e2e overall faster if we just turned off the animations at the component level instead of the consuming level
| // spinner, animated by the OS view itself — no JS, Animated, or reanimated work | ||
| // per frame. It replaces react-native-indicators, which is unmaintained and | ||
| // whose radial-transform layout collapses to a line under the New Architecture. | ||
| export const LoadingIndicator = ({ |
There was a problem hiding this comment.
Can you add a jsdoc comment explicitly saying that defaults to large and defaults to COMAPEO_BLUE?
There was a problem hiding this comment.
We should rename this to something that more obviously separates this from the LoadingIndicator.
Perhaps FullScreenCenteredLoader or something along those lines
| }: { | ||
| color?: string; | ||
| size?: number; | ||
| size?: 'small' | 'large'; |
There was a problem hiding this comment.
i dont think we should allow a size prop here. the one place where it is small is the BackgroundMapsScreen and it is almost hard to see. I think this should always be Large, and we should not even allow any sizing prop
| size = 'large', | ||
| color = COMAPEO_BLUE, | ||
| ...props | ||
| }: Props) => <ActivityIndicator size={size} color={color} {...props} />; |
There was a problem hiding this comment.
Some of the components using this are turning off the animation for the e2e tests. Should we just not turn of ALL the ActivityIndicators in e2e, instead of doing it for each disparate component? In otherwords, just control that prop directly in this. That would simplify the code and make the e2e faster all around, no?
…reen loader is large only.
Replaces the unmaintained
react-native-indicatorsspinner with React Native's built-inActivityIndicator.react-native-indicatorsis unmaintained and incompatible with the New Architecture — it was causing rendering issues and pinning CPU to 100%. A newLoadingIndicatorshared component wrapsActivityIndicatorand all call sites switch to it.Note: this is a visual change. The spinner is now the platform-native Material Design spinner on Android.
Split out of #1994 to reduce that PR's review surface; #1994 is stacked on top of this branch.