-
-
Notifications
You must be signed in to change notification settings - Fork 514
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
feat(iOS): Implement blurEffect for new architecture #2207
feat(iOS): Implement blurEffect for new architecture #2207
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, thanks for creating this PR! I left a comment about the way how we're defining that prop in a Spec.
@@ -44,6 +44,7 @@ export interface NativeProps extends ViewProps { | |||
backButtonDisplayMode?: WithDefault<BackButtonDisplayMode, 'default'>; | |||
hideBackButton?: boolean; | |||
backButtonInCustomView?: boolean; | |||
blurEffect?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if I'm fond of setting blurEffect as a string. In our types.tsx
, we're using a special type for defining every possible string that could be a blurEffect. I'm concerned if there could be a situation, where invalid blurEffect entered here could just bring a crash on iOS. Some day ago, there was that PR that was also adding a support for blurEffect on Fabric. Sadly, it has been abandoned, but maybe you could try to actually enter the type to the Spec and check if it'll just work the same as with string
?
@tboba Thanks for having a look. I will verify how incorrect values are handled. I liked the simplicity of this approach since it can use the same conversion code as the old arch, but I can also implement it using an enum, it will just require an extra function to convert from the codegen enum to the uikit one. |
Looks like there is also an android build issue I will need to fix. |
@tboba Updated to use enum and fixed android build! |
1a84a67
to
25b0101
Compare
@tboba Friendly ping on this :) |
Hey hey @janicduplessis, yeah I'll try to check that today 👍 Thanks for the nudge :D |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looks good to me! 🫡
Let's proceed when builds will pass 👍
Description
Implements blurEffect header config for new arch.
Changes
I kept this super simple and used string for codegen so we can simply reuse the RCTConvert implementation.
Test code and steps to reproduce
Tested that header blur now works in an app with new arch enabled.
Checklist