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: check for scrollEl existence #29410

Closed
wants to merge 1 commit into from
Closed

Conversation

stewones
Copy link
Contributor

@stewones stewones commented Apr 27, 2024

ion-content is breaking CI environments when the scrollable element is not reachable and the consumer needs to run content.scrollToBottom()


What is the current behavior?

N/A

What is the new behavior?

N/A

Does this introduce a breaking change?

  • Yes
  • No

Other information

TypeError: Cannot read properties of undefined (reading 'scrollHeight')

      at Content.<anonymous> (../../node_modules/@ionic/core/components/ion-content.js:242:28)
      at fulfilled (../../node_modules/tslib/tslib.js:166:62)
      at _ZoneDelegate.Object.<anonymous>._ZoneDelegate.invoke (../../node_modules/zone.js/bundles/zone.umd.js:411:30)
      at AsyncTestZoneSpec.Object.<anonymous>.AsyncTestZoneSpec.onInvoke (../../node_modules/zone.js/bundles/zone-testing.umd.js:1261:47)
      at ProxyZoneSpec.Object.<anonymous>.ProxyZoneSpec.onInvoke (../../node_modules/zone.js/bundles/zone-testing.umd.js:297:43)

ion-content is breaking CI environments when the scrollable element is not reachable, and since this can occur for many reasons it's kinda expensive to be tracking all possible consumer scenarios in a test suite.
@stewones stewones requested a review from a team as a code owner April 27, 2024 21:16
Copy link

vercel bot commented Apr 27, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
ionic-framework ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 27, 2024 9:19pm

@sean-perkins
Copy link
Contributor

Hello @stewones, can you create an issue with a minimal reproduction for me to test against?

The scroll element is set during render. I need more context to understand what issue is being experienced and why this change is the best way to resolve it.

e.g. If this issue impacts scrollToBottom, what about scrollToPoint and scrollToTop?

Thanks!

@sean-perkins sean-perkins added the needs: reply the issue needs a response from the user label Apr 29, 2024
@stewones
Copy link
Contributor Author

hey @sean-perkins
just did it #29419

@ionitron-bot ionitron-bot bot added triage and removed needs: reply the issue needs a response from the user labels Apr 29, 2024
@stewones
Copy link
Contributor Author

regarding scrollToPoint and scrollToTop, they're prob broken as well. I'm happy to patch them as well if you think this should be baked into the core.

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

Successfully merging this pull request may close these issues.

None yet

2 participants