Skip to content

fix(slides): add effect plugins for swiper #16616

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

Closed
wants to merge 17 commits into from

Conversation

CFT-Chris
Copy link
Contributor

Short description of what this resolves:

Missing Coverflow, Cube, Fade and Flip swiper effects for ion-slides

Changes proposed in this pull request:

  • Add needed plugins to swiper builder

Ionic Version: 4.0.0-beta.17

Fixes: #15857

@ionitron-bot ionitron-bot bot added the package: core @ionic/core package label Dec 6, 2018
@manucorporat
Copy link
Contributor

Can you run npm run build and commit the changes as well? also make sure you run npm i before.

@kalicki
Copy link

kalicki commented Dec 13, 2018

Tested and working, however, it is necessary to generate the build to work.

@kalicki
Copy link

kalicki commented Dec 21, 2018

Status?

@CFT-Chris
Copy link
Contributor Author

Sorry, is this waiting for something on my end? I rebuilt and committed a few weeks ago. I can do it again if needed.

@Nazirovich
Copy link

Short description of what this resolves:

Missing Coverflow, Cube, Fade and Flip swiper effects for ion-slides

Changes proposed in this pull request:

  • Add needed plugins to swiper builder

Ionic Version: 4.0.0-beta.17

Fixes: #15857

Fade effect still not working

@kalicki
Copy link

kalicki commented Feb 19, 2019

hey @manucorporat

We're out of beta and it wasn't inserted, why? :(

ping to @brandyscarney

@liamdebeasi
Copy link
Contributor

@brandyscarney I noticed we already have swiper as a dependency on our package.json: https://github.com/ionic-team/ionic/blob/master/core/package.json#L58

Do we need this additional swiper.js file?

@kalicki
Copy link

kalicki commented Feb 26, 2019

@liamdebeasi @brandyscarney Interesting, there seems to be a load by the package and its bundle separates into folder. It makes no sense this double load.

Remember that you have a new version:
https://github.com/nolimits4web/swiper/blob/master/CHANGELOG.md#swiper-450---released-on-february-22-2019

@liamdebeasi
Copy link
Contributor

liamdebeasi commented Feb 28, 2019

Yeah that's interesting. I think it may be fine for now. What do you think @brandyscarney?

Re: the new swiper version-- I think this should be a separate PR, but worth looking into!

@liamdebeasi
Copy link
Contributor

Hi there,

After some discussion with the team, we have decided to go a different route with the swiper animation plugins. Instead of adding the additional animations directly into the framework, we have decided to expose the event handlers that swiper provides. This has a few benefits:

  1. Ionic users do not need to include the swiper animations in their bundle if they aren't using it.
  2. Users can still incorporate the flip, fade, cube, and coverflow effects. We will provide instructions on how to do so in the Ionic Docs.
  3. Users can also develop and integrate their own custom animations.

I have created a PR for this here: #17959 which will be merged instead of this PR. Any feedback on this PR would be greatly appreciated!

Thank you very much for your contributions here, we really appreciate it! You will be added as a co-contributor to #17959.

Thanks!

@brandyscarney
Copy link
Member

I'm going to close this, please refer to Liam's comment above as to why. If anyone has feedback please add it on the Pull Request he linked to, thank you!

liamdebeasi added a commit that referenced this pull request Apr 16, 2019
fixes #16616

Co-Authored-By: CFT-Chris <mail@chrislo.ca>
@CFT-Chris CFT-Chris deleted the slider-swiper branch May 7, 2019 20:10
kiku-jw pushed a commit to kiku-jw/ionic that referenced this pull request May 16, 2019
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.

6 participants