-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat: System Bars Plugin #8180
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: System Bars Plugin #8180
Conversation
|
This looks great, what more is needed before merge? |
|
I am noticing that currently, without this plugin, the safe area and the status bar are working great on iOS -- it even changes the status bar text depending on light or dark mode when I toggle dark on the root: I am noticing this PR includes some styling on the status bar. Is there any way to have Android act like the iOS and have it "just work"? I also want to make sure this won't break what we have working fine on iOS. |
I am not sure if you have a correct understanding of this "inject css variables" thing. It doesn't inject |
|
I'd like to add that as of November 1st (actually earlier, but Google granted an extension to developers that depend on older SDKs) no one will be able to publish a release to the play store if they're not supporting edge-to-edge. So I think this should be given the utmost priority by the Capacitor team |
I just discovered all of this was only a problem with an Android Medium emulator. I just tried the Pixel 9 emulator and safe area is working great without any plugins. |
|
Hello everyone! Stay tuned, I'm on the verge of getting this past the finish line, The plan is to make some final changes and wrap this up this week. |
By "just work", you're expecting just the default status bar, without the fullscreen / transparent status bar style, correct? |
|
I'm curious which approach you've chosen to go with. I'm still advocating for going the native route. To demonstrate how that would work I made a simple demo proof of concept. Mind you that I've created this in like 5 minutes, so it's still not perfect, but you will get the main idea. Demo: https://github.com/tafelnl/capacitor-demo I also commented some todos which I haven't done because it's a proof of concept. |
As of right now on iOS, without this plugin, the status bar is transparent and switches the text color automatically to the right color when I set the root html class to dark mode and light mode. Not sure how this is working. |
markemer
left a comment
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.
Looks good to me, but needs an npm run fmt
|
Kicked off an update to see if that would clear the fmt issue, since you clearly ran it as the last step. |
Thanks, sounds promising! Which approach are you referring to exactly? I proposed several changes. Among which were the keyboard fix, |
|
IMO the safe area workaround and styling of system bars are two different feature sets. To me it would make more sense to include the workaround in the core. Then deprecate/remove the |
| zoomableWebView = JSONUtils.getBoolean(configJSON, "android.zoomEnabled", JSONUtils.getBoolean(configJSON, "zoomEnabled", false)); | ||
| resolveServiceWorkerRequests = JSONUtils.getBoolean(configJSON, "android.resolveServiceWorkerRequests", true); | ||
| adjustMarginsForEdgeToEdge = JSONUtils.getString(configJSON, "android.adjustMarginsForEdgeToEdge", "disable"); | ||
| adjustMarginsForEdgeToEdge = JSONUtils.getString(configJSON, "android.adjustMarginsForEdgeToEdge", "auto"); |
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 think adjustMarginsForEdgeToEdge can be removed?
| private void initSystemBars() { | ||
| String style = getConfig().getString("style", STYLE_DEFAULT).toUpperCase(Locale.US); | ||
| boolean hidden = getConfig().getBoolean("hidden", false); | ||
| boolean disableCSSInsets = getConfig().getBoolean("disableInsets", false); |
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.
With a naming like disableInsets I would expect that either 1) insets are disabled entirely (similar to adjustMarginsForEdgeToEdge: force) or 2) that the injection of custom vars is disabled. Both isn't true. I think it makes sense to include a config var like insetsCorrectionBehavior that is an enum with the following values:
disabled: this entirely disables the logic herefallbackToInjection: this enables the logic and also enables injection for older webviewsfallbackToMargins: this enables the logic and for older webviews falls back to shrinking the webview using margins (IMO this should be the default as discussed before)
Right now this PR makes it quite hard to override this behavior if a developer is - for whatever reason - looking to change the core behavior (for example by installing a plugin that has a different philosophy about this).
On another note: the disableCSSInsets is checked inside the evaluateJavascript callback. But - with the current logic - it can be lifted to check it before initSystemBars is called
| }); | ||
| } | ||
|
|
||
| @PluginMethod |
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.
The swift equivalents of the plugin methods here are annotated with a returnType: none. Would it make sense to change it to @PluginMethod(returnType = PluginMethod.RETURN_NONE)?
This PR introduces a new core plugin, System Bars, designed to be a modern take on the Status Bar plugin, managing Android System Bars, the iOS status bar, and fix issues related to broken safe area insets on Android.