Skip to content

Conversation

@EinfachHans
Copy link
Contributor

closes #22256

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?

Issue Number: #22256

Like wrote in the linked Issue, the Refresher sometimes starts too early / buggy when used in iOS card style modal. I debugged this today and saw, that the ScrollElement's offsetHeight could be zero here:

this.scrollEl = await contentEl.getScrollElement();

This cause the setupiOSNativeRefresher to not work properly, as MAX_PULL is zero then as well.

What is the new behavior?

  • Before loading the ScrollElement from the Content Element, wait for the Content's Component to be ready

Does this introduce a breaking change?

  • Yes
  • No

Other information

@liamdebeasi
Copy link
Contributor

Thanks for the PR. Can you sync your branch with the latest changes in master? There were a few changes required for the node 15/npm 7 upgrade which is why outdated branches may see failing tests.

return;
}

await contentEl.componentOnReady();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we add this here, then we should remove the componentOnReady call in https://github.com/ionic-team/ionic-framework/blob/master/core/src/components/refresher/refresher.tsx#L374 as it is redundant.

@github-actions github-actions bot added package: angular @ionic/angular package package: react @ionic/react package package: vue @ionic/vue package labels Nov 2, 2020
@EinfachHans
Copy link
Contributor Author

@liamdebeasi done

@liamdebeasi
Copy link
Contributor

Hmm it looks like it's trying to merge in all of the changes as new commits. It might be easier reset your branch to the latest master, make your change, then force push the updates.

@github-actions github-actions bot removed package: angular @ionic/angular package package: vue @ionic/vue package package: react @ionic/react package labels Nov 2, 2020
@EinfachHans
Copy link
Contributor Author

oh lol yeah i have seen it^^ did like you said: reset and the force pushed 😊

Copy link
Contributor

@liamdebeasi liamdebeasi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍

@liamdebeasi liamdebeasi merged commit 91d0414 into ionic-team:master Nov 2, 2020
@liamdebeasi
Copy link
Contributor

Merged. Thank you!

@EinfachHans EinfachHans deleted the issue-22256 branch November 2, 2020 18:16
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.

bug: ios native refresher refreshes too early when used in modal

2 participants