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.

Sign up for GitHub

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

Jump to bottom

Display custom FIAMs with SwiftUI #7496

Merged
merged 50 commits into from
Mar 16, 2021
Merged

Display custom FIAMs with SwiftUI #7496

merged 50 commits into from
Mar 16, 2021

Conversation

christibbs
Copy link
Contributor

Hey all, opening this PR up early to facilitate API review.

I'll add a test project as well-- for now, see the Code Samples section of the API review document for usage.

@google-cla google-cla bot added the cla: yes label Feb 9, 2021
@google-oss-bot
Copy link

1 Warning
⚠️ Did you forget to add a changelog entry? (Add #no-changelog to the PR description to silence this warning.)

Generated by 🚫 Danger

@google-oss-bot
Copy link

google-oss-bot commented Feb 9, 2021 •
edited

Coverage Report

Affected SDKs

  • FirebaseInAppMessaging-iOS-FirebaseInAppMessaging.framework

    SDK overall coverage changed from ? (2331dab) to 43.78% (5d8b009) by ?.

    Click to show coverage changes in 42 files.
    Filename Base (2331dab) Head (5d8b009) Diff
    FIRIAMActionURLFollower.m ? 54.31% ?
    FIRIAMActivityLogger.m ? 60.23% ?
    FIRIAMAnalyticsEventLoggerImpl.m ? 61.46% ?
    FIRIAMBannerViewController.m ? 0.00% ?
    FIRIAMBannerViewUIWindow.m ? 0.00% ?
    FIRIAMBaseRenderingViewController.m ? 0.00% ?
    FIRIAMBookKeeper.m ? 90.16% ?
    FIRIAMCardViewController.m ? 0.00% ?
    FIRIAMClearcutHttpRequestSender.m ? 0.00% ?
    FIRIAMClearcutLogStorage.m ? 69.80% ?
    FIRIAMClearcutLogger.m ? 65.03% ?
    FIRIAMClearcutUploader.m ? 92.31% ?
    FIRIAMClientInfoFetcher.m ? 59.54% ?
    FIRIAMDefaultDisplayImpl.m ? 1.60% ?
    FIRIAMDisplayCheckOnAnalyticEventsFlow.m ? 0.00% ?
    FIRIAMDisplayCheckOnAppForegroundFlow.m ? 0.00% ?
    FIRIAMDisplayCheckOnFetchDoneNotificationFlow.m ? 0.00% ?
    FIRIAMDisplayCheckTriggerFlow.m ? 0.00% ?
    FIRIAMDisplayExecutor.m ? 62.55% ?
    FIRIAMDisplayTriggerDefinition.m ? 68.42% ?
    FIRIAMElapsedTimeTracker.m ? 92.31% ?
    FIRIAMFetchFlow.m ? 81.09% ?
    FIRIAMFetchOnAppForegroundFlow.m ? 0.00% ?
    FIRIAMFetchResponseParser.m ? 86.32% ?
    FIRIAMImageOnlyViewController.m ? 0.00% ?
    FIRIAMMessageClientCache.m ? 81.44% ?
    FIRIAMMessageContentDataWithImageURL.m ? 81.50% ?
    FIRIAMMessageDefinition.m ? 100.00% ?
    FIRIAMModalViewController.m ? 0.00% ?
    FIRIAMMsgFetcherUsingRestful.m ? 71.75% ?
    FIRIAMRenderingEffectSetting.m ? 100.00% ?
    FIRIAMRenderingWindowHelper.m ? 0.00% ?
    FIRIAMRuntimeManager.m ? 0.00% ?
    FIRIAMSDKModeManager.m ? 94.12% ?
    FIRIAMSDKSettings.m ? 0.00% ?
    FIRIAMServerMsgFetchStorage.m ? 0.00% ?
    FIRIAMTimeFetcher.m ? 0.00% ?
    FIRInAppMessaging+Bootstrap.m ? 0.00% ?
    FIRInAppMessaging.m ? 3.09% ?
    FIRInAppMessagingRenderingDataClasses.m ? 42.79% ?
    NSString+FIRInterlaceStrings.m ? 100.00% ?
    UIColor+FIRIAMHexString.m ? 100.00% ?

Test Logs

paulb777
paulb777 reviewed Feb 9, 2021
View reviewed changes
FirebaseInAppMessagingSwift.podspec Outdated Show resolved Hide resolved
FirebaseInAppMessagingSwift.podspec Outdated Show resolved Hide resolved
@christibbs
Copy link
Contributor Author

This builds, but with unit tests disabled. Going to have to find a way to initialize a mock InAppMessageDisplayMessage in the unit tests, given that class's unavailable init. (See failures from 0eec2bb.)

paulb777
paulb777 reviewed Mar 5, 2021
View reviewed changes
Copy link
Member

@paulb777 paulb777 left a comment

Choose a reason for hiding this comment

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

LGTM - deferring approval to @ryanwilson for API approval

ryanwilson
ryanwilson reviewed Mar 6, 2021
View reviewed changes
Copy link
Member

@ryanwilson ryanwilson left a comment

Choose a reason for hiding this comment

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

Just the API review and a few small nits but otherwise LGTM!

FirebaseInAppMessaging/Swift/Source/CustomInAppMessageDisplayViewModifier.swift Outdated Show resolved Hide resolved
FirebaseInAppMessaging/Swift/Source/CustomInAppMessageDisplayViewModifier.swift Outdated Show resolved Hide resolved
FirebaseInAppMessaging/Swift/Source/CustomInAppMessageDisplayViewModifier.swift Show resolved Hide resolved
FirebaseInAppMessaging/Swift/Source/CustomInAppMessageDisplayViewModifier.swift Show resolved Hide resolved
FirebaseInAppMessaging/Swift/Source/CustomInAppMessageDisplayViewModifier.swift Show resolved Hide resolved
FirebaseInAppMessagingSwift.podspec Outdated Show resolved Hide resolved
FirebaseInAppMessagingSwift.podspec Outdated Show resolved Hide resolved
FirebaseInAppMessagingSwift.podspec Outdated Show resolved Hide resolved
FirebaseInAppMessagingSwift.podspec Outdated Show resolved Hide resolved
Package.swift Outdated Show resolved Hide resolved
ryanwilson
ryanwilson reviewed Mar 15, 2021
View reviewed changes
Package.swift Outdated Show resolved Hide resolved
ryanwilson
ryanwilson approved these changes Mar 16, 2021
View reviewed changes
Copy link
Member

@ryanwilson ryanwilson left a comment

Choose a reason for hiding this comment

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

Thanks for working through the comments!

@ryanwilson ryanwilson merged commit e22c7a7 into master Mar 16, 2021
@ryanwilson ryanwilson deleted the fiam-swift branch March 16, 2021 23:40
@firebase firebase locked and limited conversation to collaborators Apr 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@paulb777 paulb777 paulb777 approved these changes

@ryanwilson ryanwilson ryanwilson approved these changes

@peterfriese peterfriese Awaiting requested review from peterfriese

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@christibbs @google-oss-bot @ryanwilson @paulb777 @peterfriese