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

feat(refresher): iOS native refresher #20037

Merged
merged 48 commits into from
Dec 18, 2019
Merged

feat(refresher): iOS native refresher #20037

merged 48 commits into from
Dec 18, 2019

Conversation

liamdebeasi
Copy link
Contributor

@liamdebeasi liamdebeasi commented Dec 4, 2019

Dev build: 5.0.0-dev.201912061730.b8359e0

needs review: the contentId prop. This was added so we can translate the content to not bounce back over the refresher without interfering with the user's styling (if we translated the inner scroll, users would see a white space where the content once was).

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: #18664

What is the new behavior?

  • Added native iOS PTR behavior

Does this introduce a breaking change?

  • Yes
  • No

Other information

@ionitron-bot ionitron-bot bot added package: angular @ionic/angular package package: core @ionic/core package labels Dec 4, 2019
Copy link
Member

@brandyscarney brandyscarney left a comment

Choose a reason for hiding this comment

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

Note: discuss the API surrounding the contentId

core/src/components/refresher/refresher.tsx Outdated Show resolved Hide resolved
core/src/components/refresher/refresher.tsx Outdated Show resolved Hide resolved
core/src/components/refresher/refresher.tsx Outdated Show resolved Hide resolved
core/src/components/refresher/refresher.tsx Outdated Show resolved Hide resolved
core/src/components/refresher/refresher.tsx Outdated Show resolved Hide resolved
core/src/components/refresher/refresher.tsx Outdated Show resolved Hide resolved
core/src/components/refresher/usage/angular.md Outdated Show resolved Hide resolved
liamdebeasi and others added 7 commits December 5, 2019 11:17
Co-Authored-By: Brandy Carney <brandyscarney@users.noreply.github.com>
Co-Authored-By: Brandy Carney <brandyscarney@users.noreply.github.com>
Co-Authored-By: Brandy Carney <brandyscarney@users.noreply.github.com>
Co-Authored-By: Brandy Carney <brandyscarney@users.noreply.github.com>
Co-Authored-By: Brandy Carney <brandyscarney@users.noreply.github.com>
Co-Authored-By: Brandy Carney <brandyscarney@users.noreply.github.com>
Co-Authored-By: Brandy Carney <brandyscarney@users.noreply.github.com>
@liamdebeasi liamdebeasi marked this pull request as ready for review December 6, 2019 15:58
Copy link
Member

@brandyscarney brandyscarney left a comment

Choose a reason for hiding this comment

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

A few notes on using this:

  1. The iOS refresher doesn't work on desktop, should fallback to the non-native refresher
  2. These should be the defaults for ios: pulling-icon="lines" refreshing-spinner="lines"
  3. Look into alternatives to wrapping the content with a div containing contentId

@liamdebeasi
Copy link
Contributor Author

Ok iOS refresher now falls back to the non-native refresher on desktop. I tried playing with the native refresher on desktop Safari, but the experience just didn't really feel too great. Even the "native" refresher in News on macOS doesn't really feel smooth.

pulling-icon and refreshing-spinner both default to lines for iOS. However, when using Desktop Safari (and thus falling back to the non-native refresher) the non-native refresher looks odd as a result (it will show a paused spinner, that rotates 180 degrees so it's a little awkward). Any thoughts on this?

Still not sure about how we can get around adding the content-id prop without breaking theming for users.

@brandyscarney
Copy link
Member

@liamdebeasi Maybe we just document that Safari / browsers on desktop running iOS mode won't use the native refresher & therefore will look "off". If they want it to look nice on both they need to do platform detection to change the value?

@liamdebeasi
Copy link
Contributor Author

liamdebeasi commented Dec 9, 2019

Yeah a bunch of usage examples would probably be good here for that. I think running in iOS mode on desktop is a valid use case, so it would be ideal if it looked correct out of the box. Maybe we can have the refresher icon default to the old icon only on desktop iOS mode? Not sure if that makes things easier or more confusing to work with.

@liamdebeasi liamdebeasi merged commit 04e7c03 into master Dec 18, 2019
@liamdebeasi liamdebeasi deleted the refresh-update branch December 18, 2019 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: angular @ionic/angular package package: core @ionic/core package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants