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

fix(modal): status bar color now correct with sheet modal #25424

Merged
merged 10 commits into from
Jun 13, 2022
Merged

Conversation

liamdebeasi
Copy link
Contributor

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
    • Some docs updates need to be made in the ionic-docs repo, in a separate PR. See the contributing guide for details.
  • Build (npm run build) was run locally and any changes were pushed
  • Lint (npm run lint) has passed locally and any fixes were made for failures

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

Issue URL: resolves #20501

What is the new behavior?

  • When the card modal is open, the status bar will change to white text on a dark background. This also applies when dragging the card modal down then up.
  • When the card modal is closed, the status bar will revert to whatever setting it was on before. This also applies when dragging the card modal down.

Note: This is a Capacitor 3 only feature. Feature detection was added so errors do not occur in Cordova/Capacitor 2.

Since this functionality is dependent on a native plugin I was not able to write a test for this. 6.1.7-dev.11653592708.12aae268 is a dev build for testing.

Does this introduce a breaking change?

  • Yes
  • No

Other information

@github-actions github-actions bot added the package: core @ionic/core package label Jun 7, 2022
@liamdebeasi liamdebeasi marked this pull request as ready for review June 7, 2022 22:41
@liamdebeasi liamdebeasi requested a review from a team as a code owner June 7, 2022 22:41
@averyjohnston
Copy link
Contributor

The original issue also mentions the border radius not lining up with the screen as the animation starts. Are we skipping over that? I'd understand if it wasn't feasible to fix in a way that handles all devices.

@liamdebeasi
Copy link
Contributor Author

We are tracking that separately: #23015

@michielcrabbe96
Copy link

When you close the card modal, the status bar always reverts to the default style (not to the previous setting). So when the Status bar style is manually set to dark (but the default is the light style) it doesn't revert back to it on card modal close. Is this supposed to happen?

Nonetheless, this feature is an awesome addition!!

@liamdebeasi
Copy link
Contributor Author

Yes, this is intentional according to the Capacitor Docs for the Default status bar style: https://capacitorjs.com/docs/apis/status-bar#style.

At the moment Capacitor does not track what the previous style was. If this is a behavior you need, I recommend opening a feature request on https://github.com/ionic-team/capacitor-plugins.

As a workaround, you could wait for the ionModalDidDismiss event before manually setting the status bar style to Dark.

@michielcrabbe96
Copy link

Okay, thank you for the explanation and tip.

@EinfachHans
Copy link
Contributor

@liamdebeasi The StatusBar Plugin has a getInfo function, can't this be used to fetch the previous setting before?

@damida80
Copy link

damida80 commented Sep 19, 2022

Yes, this is intentional according to the Capacitor Docs for the Default status bar style: https://capacitorjs.com/docs/apis/status-bar#style.

At the moment Capacitor does not track what the previous style was. If this is a behavior you need, I recommend opening a feature request on https://github.com/ionic-team/capacitor-plugins.

As a workaround, you could wait for the ionModalDidDismiss event before manually setting the status bar style to Dark.

Thanks for providing this. But I see and I have three problems here:

  • If you have nested modals (you open another modal in the modal) and close the 2nd modal, the Statusbar is changed to Default (black text) and therefore to the wrong color.
  • The workaround works of course, thanks for providing this, but you have to check everywhere, whether you are closing the modal on another modal. Additionally you get an ugly flickering, because the StatusBar changes back and forth then.
  • If a user sets a darkmode preference manually within the App independently from the OS settings, the StatusBar style is also wrong after dismissing the modal.

@torgnywalin
Copy link

torgnywalin commented Oct 26, 2022

For our project this fix can introduce a bug thats worst then missing the statusbar when showing a card modal.
For us there are two scenarios:

  • App supports light / dark mode and user can select from (light, dark, follow system)
  • App is forced to be in either light or dark, regardless of users device settings.

If we set the correct status bar at app startup, this will be overriden after user opens and closes first card modal.
This will happen if:

  • user selects a theme in the app thats different from os theme.
  • our app has a dark header even when in light mode

Suggestions:

  • Option for this integration with Capacitor to be disabled (I dont think Ionic components should make calls to capacitor plugins without it being really clear to the developer.) (Could be default enabled to be backward compatible)
  • Option for for what statusbar style to revert back to (if integration with capacitor is enabled). (Could be default to default to be backward compatible)

When working in ionic-vue having a global setting where one could send in a ref for what statusbar style to revert to would be great option.

@liamdebeasi
Copy link
Contributor Author

liamdebeasi commented Oct 26, 2022

@torgnywalin Can you file an issue with a reproduction case?

@torgnywalin
Copy link

@liamdebeasi Thanks for quick reply. I tried to describe in this issue
#26173

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: core @ionic/core package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: swipeable modal does not toggle statusbar color for black background
7 participants