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

bug: page transition slides over menu when visible on larger screen #28737

Closed
3 tasks done
jaminm opened this issue Dec 19, 2023 · 8 comments · Fixed by #28745
Closed
3 tasks done

bug: page transition slides over menu when visible on larger screen #28737

jaminm opened this issue Dec 19, 2023 · 8 comments · Fixed by #28745
Labels
package: core @ionic/core package type: bug a confirmed bug report

Comments

@jaminm
Copy link

jaminm commented Dec 19, 2023

Prerequisites

Ionic Framework Version

v7.x

Current Behavior

On an iPad if you have a split pane and are using the ion-router-outlet and issue a push command in Vue, it used to just animate the router page area of the screen but now the transition slides over the entire screen including the menu. The web and android tablet builds only have a transition animation in the page section (they move up and down), it is just iPad builds that navigate over top of the menu as it pushes towards either the left or right.

Expected Behavior

This really should only transition between the two pages and not push the view over top of the menu.

Steps to Reproduce

  1. Have a split-pane project in vue with a menu using ion-router-outlet
  2. Make two views and navigate from one view to the other using a push command

Code Reproduction URL

No response

Ionic Info

Ionic:

Ionic CLI : 7.1.1 (/opt/homebrew/lib/node_modules/@ionic/cli)
Ionic Framework : @ionic/vue 7.6.1

Capacitor:

Capacitor CLI : 5.5.1
@capacitor/android : 5.5.1
@capacitor/core : 5.5.1
@capacitor/ios : 5.5.1

Utility:

cordova-res : not installed globally
native-run (update available: 2.0.0) : 1.7.4

System:

NodeJS : v20.8.0 (/opt/homebrew/Cellar/node/20.8.0/bin/node)
npm : 10.2.5
OS : macOS Unknown

Additional Information

No response

@ionitron-bot ionitron-bot bot added the triage label Dec 19, 2023
@liamdebeasi liamdebeasi added the ionitron: needs reproduction a code reproduction is needed from the issue author label Dec 19, 2023
Copy link

ionitron-bot bot commented Dec 19, 2023

Thanks for the issue! This issue has been labeled as needs reproduction. This label is added to issues that need a code reproduction.

Please reproduce this issue in an Ionic starter application and provide a way for us to access it (GitHub repo, StackBlitz, etc). Without a reliable code reproduction, it is unlikely we will be able to resolve the issue, leading to it being closed.

If you have already provided a code snippet and are seeing this message, it is likely that the code snippet was not enough for our team to reproduce the issue.

For a guide on how to create a good reproduction, see our Contributing Guide.

@ionitron-bot ionitron-bot bot removed the triage label Dec 19, 2023
@alexdabast
Copy link

alexdabast commented Dec 19, 2023

@jaminm is this your bug ?
https://stackblitz.com/edit/stackblitz-starters-aq7wc4?file=src%2Fmain.ts
I have the same since I migrated to angular 17
@liamdebeasi my stackblitz reproduce the behavior mentioned by @jaminm

@amandaejohnston amandaejohnston added triage and removed ionitron: needs reproduction a code reproduction is needed from the issue author labels Dec 20, 2023
@liamdebeasi
Copy link
Member

Thanks! I can confirm this is a bug.

@liamdebeasi liamdebeasi changed the title bug: iPad transition animation slides over split pane menu bug: page transition slides over menu when visible on larger screen Dec 20, 2023
@liamdebeasi liamdebeasi added package: core @ionic/core package type: bug a confirmed bug report labels Dec 20, 2023
@ionitron-bot ionitron-bot bot removed the triage label Dec 20, 2023
@liamdebeasi
Copy link
Member

Here's a dev build if you are interested in testing a proposed fix: 7.6.3-dev.11703103144.148eb1f6

Install example: npm install @ionic/angular@7.6.3-dev.11703103144.148eb1f6

@alexdabast
Copy link

@liamdebeasi thank you I tried the fix and it works fine for me now
When should this go live ?

@jaminm
Copy link
Author

jaminm commented Dec 21, 2023

@jaminm is this your bug ? https://stackblitz.com/edit/stackblitz-starters-aq7wc4?file=src%2Fmain.ts I have the same since I migrated to angular 17 @liamdebeasi my stackblitz reproduce the behavior mentioned by @jaminm

Yes that's it exactly.

github-merge-queue bot pushed a commit that referenced this issue Dec 21, 2023
…arger screens (#28745)

Issue number: resolves #28737

---------

<!-- Please do not submit updates to dependencies unless it fixes an
issue. -->

<!-- Please try to limit your pull request to one type (bugfix, feature,
etc). Submit multiple pull requests if needed. -->

## What is the current behavior?
<!-- Please describe the current behavior that you are modifying. -->

In #28246 we removed
the overflow on Nav and Router Outlet to allow content to flow outside
of these containers. This allows the translucent tab effect to work
(otherwise content would be clipped and there would be no translucency).
However, this had the unintended side effect of causing page transitions
to flow outside of these components. This is most noticeable on larger
displays when using SplitPane because the page can cover the menu
mid-transition.

## What is the new behavior?
<!-- Please describe the behavior or changes that are being added by
this PR. -->

- Overflow is no longer allowed on the main content. I originally set
the overflow on the router outlet/nav itself, but that caused the
translucent tab bar behavior to regress. Since this issue only happens
with split pane, I decided to apply the fix there.

## Does this introduce a breaking change?

- [ ] Yes
- [x] No

<!--
  If this introduces a breaking change:
1. Describe the impact and migration path for existing applications
below.
  2. Update the BREAKING.md file with the breaking change.
3. Add "BREAKING CHANGE: [...]" to the commit description when merging.
See
https://github.com/ionic-team/ionic-framework/blob/main/.github/CONTRIBUTING.md#footer
for more information.
-->


## Other information

<!-- Any other information that is important to this PR such as
screenshots of how the component looks before and after the change. -->

### Dev build
`7.6.3-dev.11703103144.148eb1f6`

⚠️ Please test in a physical app too

### Before/After Demo

| `main` | branch |
| - | - |
| <video
src="https://github.com/ionic-team/ionic-framework/assets/2721089/2f3f0d74-9a7e-4ebe-b58a-6e1a6ea3636e"></video>
| <video
src="https://github.com/ionic-team/ionic-framework/assets/2721089/cdbf8fb5-e217-48cf-8c1e-4bdecee4de4c"></video>
|

### Screenshot Diffs

The `menu-custom` screenshot diffs
([Example](https://github.com/ionic-team/ionic-framework/pull/28745/files))
are correct. By adding `overflow: hidden`, the box shadow from the
"Content" header no longer flows outside of the container.

The menu [border on MD has an opacity of
0.18](https://github.com/ionic-team/ionic-framework/blob/e5226016a0f0b066a7bd7fc9997f905d3b87fbc4/core/src/components/menu/menu.md.vars.scss#L7),
so the border color was mixing with the color of the box shadow from the
header.

Since the shadow no longer flows outside of the container, the border
color does not mix with the box shadow color.

### Does this break translucent tabs when the split pane is inside of a
tab?

No. The [split pane has `contain: strict`
set](https://github.com/ionic-team/ionic-framework/blob/e5226016a0f0b066a7bd7fc9997f905d3b87fbc4/core/src/components/split-pane/split-pane.scss#L24)
which prevents content from flowing under the tab bar, so the
translucent tab bar never worked with this layout.

---------

Co-authored-by: ionitron <hi@ionicframework.com>
@liamdebeasi
Copy link
Member

Thanks for the issue. This has been resolved via #28745, and a fix will be available in an upcoming release of Ionic Framework.

Copy link

ionitron-bot bot commented Jan 20, 2024

Thanks for the issue! This issue is being locked to prevent comments that are not relevant to the original issue. If this is still an issue with the latest version of Ionic, please create a new issue and ensure the template is fully filled out.

@ionitron-bot ionitron-bot bot locked and limited conversation to collaborators Jan 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
package: core @ionic/core package type: bug a confirmed bug report
Projects
None yet
4 participants