Skip to content

Let users override "useNativeDriver" for Shimmer / ActivityIndicator - #1148

Closed
Saad Najmi (Saadnajmi) wants to merge 16 commits into
microsoft:masterfrom
Saadnajmi:useNativeDriver
Closed

Saad Najmi (Saadnajmi) wants to merge 16 commits into
microsoft:masterfrom
Saadnajmi:useNativeDriver

Conversation

@Saadnajmi

@Saadnajmi Saad Najmi (Saadnajmi) commented Nov 10, 2021 •

Copy link
Copy Markdown
Collaborator

Platforms Impacted

  • iOS
  • macOS
  • win32 (Office)
  • windows
  • android

Description of changes

A client team would like to use Shimmer & ActivityIndicator. However, we would want to make sure we set useNativeDriver for those applications properly to ensure the best performance based on their react-native version. Due to a bug with animations (fixed with react/react-native#29585 and microsoft/react-native-macos#763), we can't set the useNativeDriver prop to true unless we're on RN 0.66+ or RN-macOS 0.62+.

Let's add a check that sets useNativeDriver to it's proper value based on our RN version, and also add useNativeDriver as a prop to Shimmer and ActivityIndicator, should the client need to override it for whatever reason (lex: they patch the bug in their own repo, or update to a newer version of react-native).

Verification

Ran the iOS and macOS test app with console.logs of the what value useNativeDriver is set to, to make sure it was set properly.

Pull request checklist

This PR has considered (when applicable):

  • Automated Tests
  • Documentation and examples
  • Keyboard Accessibility
  • Voiceover
  • Internationalization and Right-to-left Layouts

Comment on lines +49 to +56
const nativeVersion = Platform.constants.reactNativeVersion;
if (Platform.OS === 'macos' && nativeVersion.minor >= 62) {
return true;
} else if (nativeVersion.minor >= 66) {
return true;
} else {
return false;
}

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't a perfect check of whether we are running with react-native-macos or not: An iOS or Android app could be running with react-native-macos 0.63, and this would return false where it should return true. I couldn't think of a better way to do it though.

Comment thread packages/experimental/Shimmer/src/Shimmer.tsx Outdated
/**
* overrides the useNativeDriver prop on the component's animation
*/
useNativeDriver?: boolean;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we not expose it as a prop if we do the RN version check inside the component?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the prop in case, say, a user is using some other fork of react-native or managed to patch react-native to include that fix, so they could override our default value.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like we are trying to cover all use cases rather than take it one step at a time. I personally think exposing this as a prop can be risky as it could affect performance of the control.

@Saadnajmi

Copy link
Copy Markdown
Collaborator Author

Talked offline, and I'm closing this in favor of a different solution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants