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(refresher): ios native refresher now works in side menu #22449

Merged
merged 3 commits into from Nov 10, 2020

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)
  • 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?

Sometimes the scrollEl's clientHeight is zero because the containing element is hidden. This results in the refresher firing too early.

What is the new behavior?

  • Added a default MAX_PULL value in the event the content height is 0.

Does this introduce a breaking change?

  • Yes
  • No

Other information

@EinfachHans
Copy link
Contributor

The title of this PR is not correct i think 😃 And what about assigning MAX_PULL later? Where we knew the scrollEl must be visible. For example when the gesture starts?

@liamdebeasi
Copy link
Contributor Author

Our PR titles and commit messages are designed to explain a concrete bug that a commit fixes, so just saying "ensure MAX_PULL is not 0" is not enough. In this case, this particular bug was discovered in a menu, so the title is going to reference the menu.

@liamdebeasi
Copy link
Contributor Author

I'd rather not do DOM reads every time the user puts their finger/pointer down on the screen, but I can look into an alternative.

@EinfachHans
Copy link
Contributor

Of course a fixed value is also not bad, but currently i like that it is based on the scroll height 😃 Maybe just save the scroll height on the first time the gesture starts if this is an option

@liamdebeasi liamdebeasi merged commit a4a6453 into master Nov 10, 2020
@liamdebeasi liamdebeasi deleted the ref-height branch November 10, 2020 17:07
TakumaKira pushed a commit to TakumaKira/ionic-framework that referenced this pull request Nov 13, 2020
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.

None yet

2 participants