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: ion-header fade/condense usage is incorrect with Angular CDK Virtual Scroll #25463

Closed
4 of 7 tasks
mtnair opened this issue Jun 14, 2022 · 6 comments
Closed
4 of 7 tasks
Assignees
Labels
package: core @ionic/core package type: bug a confirmed bug report

Comments

@mtnair
Copy link

mtnair commented Jun 14, 2022

Prerequisites

Ionic Framework Version

  • v4.x
  • v5.x
  • v6.x
  • Nightly

Current Behavior

When using the <ion-header> component in combination with Angular's <cdk-virtual-scroll-viewport> component, to fade/condense the header when scrolling in the list, it does not work. I also added the ion-content-scroll-host class to the <cdk-virtual-scroll-viewport> component to no avail.

I created a demo with the header effect inspired by the workaround proposed in the feature request #25157:
https://stackblitz.com/edit/ionic6-angular13-rsezrm?file=src%2Fapp%2Fapp.component.ts

If you set virtualScroll in the app.component.ts to "false", you can see the expected behavior without a virtual scroll.

Another problem in the current behavior: When setting the height of the <cdk-virtual-scroll-viewport> element to 100%, the space used by the <ion-header> inside <ion-content> causes to overflow the virtual scroll component, so the last element in the list is invisible.

Expected Behavior

When using the <ion-header> component in combination with Angular's <cdk-virtual-scroll-viewport> component, the header should fade and condense like it's described in the official documentation: https://ionicframework.com/docs/api/header#usage-with-virtual-scroll

Steps to Reproduce

  1. Run the Stackblitz Demo application: https://ionic6-angular13-rsezrm.stackblitz.io/ OR clone the repo: https://github.com/mtnair/ionic6-virtual-scroll-header-repro
  2. Scroll down
  3. See the header behavior with the virtual scroll component
  4. Change the param virtualScroll in app.component.ts to "false"
  5. Scroll down
  6. See the expected behavior with an <ion-list> without the virtual scroll

Code Reproduction URL

https://github.com/mtnair/ionic6-virtual-scroll-header-repro

Ionic Info

$ ionic info

Ionic:

Ionic CLI : 6.18.2 (C:\Users[...]\AppData\Roaming\npm\node_modules@ionic\cli)
Ionic Framework : @ionic/angular 6.1.9
@angular-devkit/build-angular : 13.3.7
@angular-devkit/schematics : 13.3.7
@angular/cli : 13.3.7
@ionic/angular-toolkit : 6.1.0

Utility:

cordova-res : 0.15.4
native-run : 1.5.0

System:

NodeJS : v14.17.5 (C:\Program Files\nodejs\node.exe)
npm : 8.10.0
OS : Windows 10

Additional Information

I followed the docs on the implementation of the virtual scrolling.

A related issue that I came across: #25318

@ionitron-bot ionitron-bot bot added the triage label Jun 14, 2022
@sean-perkins
Copy link
Contributor

@mtnair thanks for reporting this issue!

If you relocate the ion-header inside of the virtual scroll viewport, it behaves as expected:

I need to revisit the implementation. The test case has ion-header inside the virtual scroll viewport, but the docs have it outside of it.

@sean-perkins sean-perkins self-assigned this Jun 14, 2022
@mtnair
Copy link
Author

mtnair commented Jun 16, 2022

Thank you, with the ion-header inside the cdk-virtual-scroll-viewport it behaves as expected. Although its a little bit weird, why this component needs to be inside the virtual scroll element, as other Ionic components like ion-refresher don't need to be inside it.

@sean-perkins sean-perkins changed the title bug: ion-header doesn't work in combination with Angular's virtual scroll component bug: ion-header fade/condense usage is incorrect with Angular CDK Virtual Scroll Oct 18, 2022
@sean-perkins sean-perkins added package: core @ionic/core package type: bug a confirmed bug report and removed triage labels Oct 18, 2022
@sean-perkins
Copy link
Contributor

Apologies for the delay. I have been able to diagnose this issue further and believe the current usage in the docs is correct, but the current implementation pattern is incorrect. The ion-header element should not be within the virtual scroll's scroll container.

When originally implementing this, I did not account for the intersection observer that is used for the fade and collapsible effect. I will need to translate the header out-of-view when using a virtual scroll container. Also, some of the queries for DOM elements will need to be adjusted to query from ion-content instead of our custom scroll target selector.

I have a work-in-progress branch that I will create a dev build from, once I verify the condense and fade effects are consistent with non-virtual scroll usage.

@sean-perkins
Copy link
Contributor

Here is a dev-build to test with:

npm install @ionic/[email protected]

One thing to note, when using a virtual scroll container with the fade/condense effect, the dev build will now translate the header off the view. Since the scroll element is disabled for the ion-content, the header does not have the same overflow rules.

To have the same design as when used without virtual scrolling, wrap the content in a container with overflow: hidden.

e.g.:

#container {
  height: 100%;
  overflow: hidden;
}
<ion-header></ion-header>
<ion-content [scrollY]="false">
  <div id="container">
    <ion-header></ion-header>
    <cdk-virtual-scroll class="ion-content-scroll-host"></cdk-virtual-scroll>
  </div>
</ion-content>

Without this, the header that is scrolled off the view will show blurred under the condensed header.

@sean-perkins
Copy link
Contributor

Hello after looking at this issue fresh and comparing against the proposed changes, I have decided to close this out.

The collapse header implementation requires to be rendered inside of a scroll container, so that the header can be moved off the visible viewport and the intersection observer can fire to render the condensed version. When using virtual scroll and the custom scroll target, the ion-content is no longer the scrollable container. This means that the correct usage is to render the ion-header inside of the .ion-content-scroll-host container (the virtual scroll container). This aligns with my original comments when triaging the issue.

I think we can improve the documentation around this, by showing usage for condense in the docs as well. I have created: ionic-team/ionic-docs#2663 to track this.

@sean-perkins sean-perkins closed this as not planned Won't fix, can't repro, duplicate, stale Dec 2, 2022
@ionitron-bot
Copy link

ionitron-bot bot commented Jan 1, 2023

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 1, 2023
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
Development

No branches or pull requests

2 participants