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

chore(ios): exposed new constants of blur effect style #11117

Merged
merged 3 commits into from Aug 29, 2019

Conversation

vijaysingh-axway
Copy link
Contributor

@build build added this to the 8.2.0 milestone Aug 6, 2019
@build build requested review from a team August 6, 2019 19:42
@build
Copy link
Contributor

build commented Aug 6, 2019

Fails
🚫 Tests have failed, see below for more information.
Messages
📖

💾 Here's the generated SDK zipfile.

📖 ❌ 1 tests have failed There are 1 tests failing and 472 skipped out of 4368 total tests.
📖 ✊ The commits in this PR match our conventions! Feel free to Rebase and Merge this PR when ready.

Tests:

ClassnameNameTimeError
android.emulator.Titanium.Apppause/resume events5.105
Error: timeout of 5000ms exceeded
at Titanium.<anonymous> (/ti-mocha.js:4290:23)

Generated by 🚫 dangerJS against 674548a

@@ -443,6 +443,24 @@ - (id)BLUR_EFFECT_STYLE_PROMINENT
}
return [NSNull null];
}

#if IS_SDK_IOS_13
MAKE_SYSTEM_PROP(BLUR_EFFECT_STYLE_SYSTEM_ULTRA_THIN_MATERIAL, UIBlurEffectStyleSystemUltraThinMaterial);
Copy link
Contributor

Choose a reason for hiding this comment

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

These new properties are missing a check when running on devices with iOS < 13.. See https://jira.appcelerator.org/browse/TIMOB-23778 and the above BLUR_EFFECT_STYLE_* properties.

Maybe this is good chance to extend the MAKE_SYSTEM_PROP macro and add an additional parameter to do the above runtime check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@janvennemann
If we are building with iOS SDK 13, these all constants will be available in in iOS 13+ and iOS < 13. But for iOS < 13 ( [[self blurView] setEffect:[UIBlurEffect effectWithStyle:style]];) setEffect native API internally take care of it and will not add any effect.
If we are building with iOS SDK < 13, in that case these constants will not be available and will give null because constants are wrapped inside macro.

I have tested this.
So we do not have to add any check for that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alright, thanks for clearing this up. I saw the way the other properties are implemented and was wondering if this affects the new properties as well. If you say the check isn't needed then this looks good!

@ssjsamir ssjsamir self-requested a review August 29, 2019 14:25
Copy link
Contributor

@ssjsamir ssjsamir left a comment

Choose a reason for hiding this comment

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

FR Passed Able to see that new constants are exposed for blur styling effects. Tested with the test case from:https://jira.appcelerator.org/browse/TIMOB-27310

Test Environment

MacOS Mojave version 10.14.4
Xcode 11 beta 5
Node.js ^8.11.1
iPhone 8 plus sim (13.0)
"NPM":"4.2.14","CLI":"7.1.1-master.2"

@sgtcoolguy sgtcoolguy merged commit 50f5435 into tidev:master Aug 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants