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

feat(android): Add Statusbar.setOverlaysWebView method #2597

Merged
merged 10 commits into from
Mar 19, 2020

Conversation

Crylion
Copy link
Contributor

@Crylion Crylion commented Mar 18, 2020

Addition of a new method in the core Statusbar plugin, similar to the overlaysWebView method/preference of the cordova statusbar plugin. Updated example in the docs for the Statusbar plugin.

Works only for android (as discussed in #1876) since on iOS it already handles it correctly on devices with notches.

I'm not confident in my android knowledge these days, so I might have overlooked something. But in my tests everything worked as expected, similar to the cordova plugin.

What I'm not sure about is the correct way to handle that the iOS version of the plugin doesn't implement this feature. I added it with a "not implemented" response in the Statusbar.swift file but I'm not sure if that is all that's needed here.

Copy link
Member

@jcesarmobile jcesarmobile left a comment

Choose a reason for hiding this comment

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

I have not tested yet, but I find the enabled name confusing, maybe just call it overlay.

Also, getInfo method should return if it's overlaid or not.

And there is a typo on Display content unter (should be under)

As I've not tested, I'm not 100% sure, but since you set the color to transparent, if you disable it, it might lose the previous color, right? I think the previous color should be restored.

@Crylion
Copy link
Contributor Author

Crylion commented Mar 18, 2020

@jcesarmobile Those are good points that I also thought about. 👍
But it also seemed strange to me to just mirror the name of the method and call the option "overlayWebview" 😕
If I'm being honest, I would have preferred to not use an object at all and just take a boolean as a parameter, but looking at all the other official plugins I could not find any example of that 🤔
I'll go with your suggestion and call it overlay.

About the color: I also was not really satisfied with the way it is, but the cordova plugin actually does the same. I suspect that this is because it would be unusual to actually switch the overlaying behavior of the status bar back and forth during runtime? But I share your opinion that this isn't very clean that way. I'll look at restoring it!

(and i'm obviously fixing the typo ;D)

Thanks for your time!

Copy link
Member

@jcesarmobile jcesarmobile left a comment

Choose a reason for hiding this comment

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

Tested and works fine.
Requested a few minor changes.

On iOS you also have to add the method here
https://github.com/ionic-team/capacitor/blob/master/ios/Capacitor/Capacitor/Plugins/DefaultPlugins.m#L138

@@ -1633,6 +1638,11 @@ export interface StatusBarInfoResult {
visible: boolean;
style: StatusBarStyle;
color?: string;
overlaysWebview: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

Change the name to overlaid or overlays, and make it optional because iOS doesn't return it

Suggested change
overlaysWebview: boolean;
overlaid?: boolean;


// Display content under transparent status bar (Android only)
Statusbar.setOverlaysWebView({
enabled: true
Copy link
Member

Choose a reason for hiding this comment

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

forgot to change the new name here

Suggested change
enabled: true
overlay: true

// Display content under transparent status bar (Android only)
Statusbar.setOverlaysWebView({
enabled: true
})
Copy link
Member

Choose a reason for hiding this comment

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

Missing ; at the end

Suggested change
})
});

@@ -114,6 +124,38 @@ public void getInfo(final PluginCall call) {
data.put("visible", (decorView.getSystemUiVisibility() & View.SYSTEM_UI_FLAG_FULLSCREEN) != View.SYSTEM_UI_FLAG_FULLSCREEN);
data.put("style", style);
data.put("color", String.format("#%06X", (0xFFFFFF & window.getStatusBarColor())));
data.put("overlaysWebview", (decorView.getSystemUiVisibility() & View.SYSTEM_UI_FLAG_LAYOUT_FULLSCREEN) == View.SYSTEM_UI_FLAG_LAYOUT_FULLSCREEN);
Copy link
Member

Choose a reason for hiding this comment

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

Change the name to overlaid or overlays

Suggested change
data.put("overlaysWebview", (decorView.getSystemUiVisibility() & View.SYSTEM_UI_FLAG_LAYOUT_FULLSCREEN) == View.SYSTEM_UI_FLAG_LAYOUT_FULLSCREEN);
data.put("overlaid", (decorView.getSystemUiVisibility() & View.SYSTEM_UI_FLAG_LAYOUT_FULLSCREEN) == View.SYSTEM_UI_FLAG_LAYOUT_FULLSCREEN);

@Crylion
Copy link
Contributor Author

Crylion commented Mar 19, 2020

Hopefully all done now 🙂
I went with overlays for the info attribute because I felt it would be clearer that the status bar is the one that overlays something rather than being overlaid itself.

Thank you for your time and help!

@jcesarmobile jcesarmobile changed the title Statusbar: "overlayswebview" option for Android, similar to cordova-plugin-statusbar feat(android): Add Statusbar.setOverlaysWebView method Mar 19, 2020
Copy link
Member

@jcesarmobile jcesarmobile left a comment

Choose a reason for hiding this comment

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

looks good, thanks for making all the requested changes so fast!

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