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

Enable no margin IAMs #984

Merged
merged 3 commits into from
Sep 2, 2021
Merged

Enable no margin IAMs #984

merged 3 commits into from
Sep 2, 2021

Conversation

emawby
Copy link
Contributor

@emawby emawby commented Aug 24, 2021

Current IAMs always include a non-zero margin meaning that IAMs cannot fill the entire screen. To enable full screen IAMs we will now optionally disable the margin based on new data being sent as part of the loadHTMLContent response for an IAM. This data will come from a new "styles" dictionary in the following keys:
top_margin: 0
bottom_margin: 0
left_margin: 0
right_margin: 0

If "styles" is absent, or it does not contain a margin key that margin will keep the default margin value.

For now the SDK will only respect a value of 0. Any other value will result in the default margins. In the future we can use values from these keys to set customized margin values.


This change is Reviewable

Copy link
Member

@jkasten2 jkasten2 left a comment

Choose a reason for hiding this comment

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

What do you think about changing these new margin fields to a boolean, and with a matching name? Example remove_top_margin: true

Reviewed 1 of 1 files at r1.
Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @emawby, @Jeasmine, @jkasten2, and @nan-li)


iOS_SDK/OneSignalSDK/Source/OSInAppMessageViewController.m, line 248 at r1 (raw file):

        let baseUrl = [NSURL URLWithString:OS_IAM_WEBVIEW_BASE_URL];
        NSMutableDictionary *fakeData = [[NSMutableDictionary alloc] initWithDictionary:data];
        fakeData[@"styles"] = @{

The wording fake is something you would use in tests but this is part of the source. I would suggest something like default.


iOS_SDK/OneSignalSDK/Source/OSInAppMessageViewController.m, line 269 at r1 (raw file):

        // We are currently only allowing default margin or no margin.
        // If we receive a number that isn't 0 we want to use default margin for now.
        if (data[@"styles"][@"top_margin"]) {

Would be cleaner if we extracted data[@"styles"] into a variable we so don't have to repeat it here.

Copy link
Member

@jkasten2 jkasten2 left a comment

Choose a reason for hiding this comment

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

The boolean effect on number values is deceiving. Extensibility wise new fields could always be added later to provide number values.

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @emawby, @Jeasmine, @jkasten2, and @nan-li)

Copy link
Member

@jkasten2 jkasten2 left a comment

Choose a reason for hiding this comment

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

Also expanding on the fields on styles, it seems the code only supports centering so we should only have two fields to match, margin width and margin height.

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @emawby, @Jeasmine, @jkasten2, and @nan-li)

Copy link
Contributor Author

@emawby emawby left a comment

Choose a reason for hiding this comment

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

Do you think we should change the JSON or keep the JSON the same and change the client? With this approach I was hoping we wouldn't need to change the JSON in the future.

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @Jeasmine, @jkasten2, and @nan-li)


iOS_SDK/OneSignalSDK/Source/OSInAppMessageViewController.m, line 248 at r1 (raw file):

Previously, jkasten2 (Josh Kasten) wrote…

The wording fake is something you would use in tests but this is part of the source. I would suggest something like default.

This was used during my testing but shouldn't be in my final commit. I am assuming you are looking at the changes commit by commit so this should be removed already.


iOS_SDK/OneSignalSDK/Source/OSInAppMessageViewController.m, line 269 at r1 (raw file):

Previously, jkasten2 (Josh Kasten) wrote…

Would be cleaner if we extracted data[@"styles"] into a variable we so don't have to repeat it here.

Done.

@emawby emawby requested a review from jkasten2 August 30, 2021 16:39
Copy link
Contributor Author

@emawby emawby left a comment

Choose a reason for hiding this comment

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

updated to use height and width margin boolean

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @Jeasmine, @jkasten2, and @nan-li)

Copy link
Member

@jkasten2 jkasten2 left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Jeasmine and @nan-li)


iOS_SDK/OneSignalSDK/Source/OSInAppMessageViewController.m, line 248 at r1 (raw file):

Previously, emawby (Elliot Mawby) wrote…

This was used during my testing but shouldn't be in my final commit. I am assuming you are looking at the changes commit by commit so this should be removed already.

Ah sorry my bad.

@emawby emawby merged commit c98b1f8 into main Sep 2, 2021
@emawby emawby deleted the feature/IAM_margin_control branch September 2, 2021 17:09
@emawby emawby mentioned this pull request Sep 13, 2021
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