fix(action-sheet): background includes safe area margin #24700
Conversation
| @@ -67,7 +67,7 @@ | |||
|
|
|||
| .action-sheet-wrapper { | |||
| @include position(null, 0, 0, 0); | |||
| @include margin(var(--ion-safe-area-top, 0), auto, var(--ion-safe-area-bottom, 0), auto); | |||
| @include margin(var(--ion-safe-area-top, 0), auto, 0, auto); | |||
There was a problem hiding this comment.
This will break the safe area for iOS. The action sheet on iOS should move above the safe area, while the MD action sheet's content should move above the safe area, but the container should extend below the safe area.
I think what was wrong in my previous patch was that iOS safe area should use margin, but MD safe area should use padding.
It might make sense to revert https://github.com/ionic-team/ionic-framework/pull/24176/files and then add
.action-sheet-wrapper {
@include padding(var(--ion-safe-area-top, 0), auto, var(--ion-safe-area-bottom, 0), auto);
}
Or something to account for the safe area in MD mode
There was a problem hiding this comment.
Good catch 👍 Although the padding/margin cannot be applied to the action-sheet-wrapper in MD, as it will increase the container size (good), but won't increase the container space the background color is applied to (bad).
Likely we will have to do specific styles in iOS vs. MD stylesheets. As iOS pushes the space from the bottom, but doesn't have a background applied.
|
Still need to verify on device, will re-request reviews after I get a chance to do that. Few meetings first ⏳ |
|
Ok, confirmed this maintains iOS visual behavior and fixes the MD display. We may want to consider visual regression tests in the future that simulate safe areas 🤔 |
|
We should already have a safe area test: https://github.com/ionic-team/ionic-framework/blob/main/core/src/components/action-sheet/test/basic/index.html#L50-L55 The visual regression URL isn't super visible anymore, but here's the diff for your latest commit: https://screenshot.ionicframework.com/7315e01/bf6bf00 |
|
Oh scratch that, we don't have an E2E test that actually opens the action sheet with added safe area. Could you add one? |
@liamdebeasi looks like the |
Pull request checklist
Please check if your PR fulfills the following requirements:
npm run build) was run locally and any changes were pushednpm run lint) has passed locally and any fixes were made for failuresPull request type
Please check the type of change your PR introduces:
What is the current behavior?
The safe area is being applied to the action sheet container, which does not have the background appearance applied to it. This pushes the container up, but leaves an empty space between the last group in the action sheet and the bottom bevel of the screen.
Issue Number: #24699
What is the new behavior?
Safe are padding is applied to the last group of the action sheet, which has the background appearance applied to it. This will extend the container's visual appearance to the bottom bevel on an Android device.
Does this introduce a breaking change?
Other information