Skip to content

fix(refresher): scroll styles are reset when using non-native refresher - #27602

Merged
liamdebeasi merged 3 commits into
ionic-team:mainfrom
mrahn24:bug/refresher-hold
Jun 7, 2023
Merged

fix(refresher): scroll styles are reset when using non-native refresher#27602
liamdebeasi merged 3 commits into
ionic-team:mainfrom
mrahn24:bug/refresher-hold

Conversation

@mrahn24

@mrahn24 mrahn24 commented Jun 6, 2023

Copy link
Copy Markdown
Contributor

Issue number: resolves #27601


What is the current behavior?

The current behavior restores overflow styles while moving (within the setCSS function).

What is the new behavior?

Overflow styles are restored when refresher gesture ends.

Does this introduce a breaking change?

  • Yes
  • No

Honestly, I don't know exactly. From code perspective I would say 'Yes', but I can't get the impact of the change.

Ionic Team edit: There are no changes to the public API, and this is fixing a behavior that used to work so there are no breaking changes.

Other information

@mrahn24
mrahn24 requested a review from liamdebeasi as a code owner June 6, 2023 10:33
@bolt-new-by-stackblitz

Copy link
Copy Markdown

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@github-actions github-actions Bot added the package: core @ionic/core package label Jun 6, 2023

@liamdebeasi liamdebeasi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I made some changes to your PR. The original fix worked, but the restoreOverflowStyle was happening before the setTimeout callback fired in close(). This prevented some inline transition styles from being cleared properly. It did not cause any functional issues that I was aware of, but we should still make sure we are cleaning up styles when we no longer need them.

The revised fix causes the styles to be cleared once the setTimeout callback fires. I also added a case for when the pull gesture is reset.

@liamdebeasi liamdebeasi changed the title fix(refresher): scroll was broken when using non-native refresher fix(refresher): scroll styles are reset when using non-native refresher Jun 6, 2023
@liamdebeasi
liamdebeasi requested a review from sean-perkins June 6, 2023 16:04
@sean-perkins

Copy link
Copy Markdown
Contributor

I'm observing this behavior testing on device, specifically if you cancel the gesture, the first attempt to scroll prevents the scroll and activates the item. However, attempting to scroll after does work as expected.

Is this expected?

RPReplay_Final1686167147.MP4

@sean-perkins sean-perkins left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We chatted in Slack, the behavior I observed is from:

setTimeout(() => {
this.state = RefresherState.Inactive;
this.progress = 0;
this.didStart = false;
this.setCss(0, '0ms', false, '');
}, 600);
and is existing behavior that is independent of these changes.

Great work both of you!

@liamdebeasi
liamdebeasi added this pull request to the merge queue Jun 7, 2023
Merged via the queue into ionic-team:main with commit 92c5545 Jun 7, 2023
liamdebeasi added a commit that referenced this pull request Jun 7, 2023
…er (#27602)

Issue number: resolves #27601 

---------

## What is the current behavior?
The current behavior restores overflow styles while moving (within the
setCSS function).

## What is the new behavior?
Overflow styles are restored when refresher gesture ends.

## Does this introduce a breaking change?
- [ ] Yes
- [x] No

<!-- If this introduces a breaking change, please describe the impact
and migration path for existing applications below. -->
Honestly, I don't know exactly. From code perspective I would say 'Yes',
but I can't get the impact of the change.

Ionic Team edit: There are no changes to the public API, and this is
fixing a behavior that used to work so there are no breaking changes.

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

---------

Co-authored-by: Liam DeBeasi <liamdebeasi@users.noreply.github.com>
@mrahn24
mrahn24 deleted the bug/refresher-hold branch June 12, 2023 06:14
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: non-native refresher breaks scrolling

3 participants