Skip to content
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

[blur] explicitly pass down props on iOS #10648

Merged
merged 4 commits into from Dec 4, 2020
Merged

Conversation

cruzach
Copy link
Contributor

@cruzach cruzach commented Oct 12, 2020

Why

closes #7290

the component could sometimes be cloned (for example, when wrapped within a TouchableWithoutFeedback) and have props passed to it that it does not expect

It looks from the issue that this isn't a problem on Android but to match the expected behavior I think we might want to make the same changes to BlurView.android.tsx

How

pass down only the props the NativeBlurView expects

Test Plan

yarn test and tested on the example in the docs https://docs.expo.io/versions/latest/sdk/blur-view/#usage

@cruzach
Copy link
Contributor Author

cruzach commented Oct 13, 2020

Will take a look at iOS test suite, not exactly sure why this is failing 🤔

Copy link
Contributor

@sjchmiela sjchmiela left a comment

Choose a reason for hiding this comment

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

Shouldn't we also update BlurProps definition in BlurView.types.ts so that TS doesn't lead people into believing BlurView supports props other than tint, intensity and style?

export type BlurProps = {
tint: BlurTint;
intensity: number;
} & React.ComponentProps<typeof View>;

Plus, we depend on other props being passed to native view in other places (namely children):

function Hint({ children }: { children: string }) {
return (
<BlurView style={styles.hint} intensity={100} tint="dark">
<Text style={styles.headerText}>{children}</Text>
</BlurView>
);
}

<StyledBlurView style={{ flex: 1 }}>
{experience ? (
<ScrollView style={{ flex: 1 }} contentContainerStyle={{ paddingVertical: 48 }}>
<ExperienceContents experience={experience} />
</ScrollView>
) : (
<View style={{ flex: 1, justifyContent: 'center', alignItems: 'center' }}>
<ActivityIndicator size="large" />
</View>
)}
<ModalHeader />
</StyledBlurView>

{/* Adjust the tint and intensity */}
<BlurView intensity={100} style={[StyleSheet.absoluteFill, styles.nonBlurredContent]}>
<Text>Hello! I am bluring contents underneath</Text>
</BlurView>

@sjchmiela
Copy link
Contributor

Maybe we should wrap NativeBlurView in View and divide props into:

  • BlurProps being passed to NativeBlurView
  • ...rest props passed View?

🤔

@github-actions
Copy link
Contributor

github-actions bot commented Dec 4, 2020

Native Component List for this branch is ready

@brentvatne brentvatne merged commit 6526d26 into master Dec 4, 2020
@brentvatne brentvatne deleted the @cruzach/blurviewprpos branch December 4, 2020 21:47
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.

[expo-blur] Blur no longer working and prop warnings after upgrade to expo 36/expo-blur 8
3 participants