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(slides): expose interface to provide custom animations #17959

Merged
merged 17 commits into from Apr 16, 2019

Conversation

4 participants
@liamdebeasi
Copy link
Member

commented Apr 4, 2019

Short description of what this resolves:

This PR exposes the interface to allow users to tap into certain callbacks in Swiper. The result is that users can provide their own custom animations, allowing them to use fade, cube, flip, coverflow, etc, without us having to introduce the code into the framework code base.

Changes proposed in this pull request:

  • Ensure that users can tap into swiper callbacks but not override the callbacks Ionic uses to make ion-slides work
  • Added documentation on how to add custom animations
  • Add demos for all 4 additional animations
  • Fixed usage for slides

Fixes: #17424

This PR would allow #16616 to be closed as well.

liamdebeasi added some commits Apr 4, 2019

@ionitron-bot ionitron-bot bot added the package: core label Apr 4, 2019

liamdebeasi added some commits Apr 4, 2019

@liamdebeasi liamdebeasi added this to In progress 🤺 in Ionic Core via automation Apr 4, 2019

@kalicki

This comment has been minimized.

Copy link

commented Apr 5, 2019

@liamdebeasi

This comment has been minimized.

Copy link
Member Author

commented Apr 5, 2019

Can you post the code you are using for that? Thanks!

@kalicki

This comment has been minimized.

Copy link

commented Apr 5, 2019

Sorry =/
Actually this is with the current slides of v4.2. Can you guarantee that this does not happen in your PR ?!

I'll test your PR

@kalicki

This comment has been minimized.

Copy link

commented Apr 5, 2019

Simple code:

<ion-slides pager="false" [options]="slideOptions" #tagSlides>
        <ion-slide *ngFor="let tag of tags">
          <ion-card class="tag-image" mode="ios" (click)="goPage(tag)">
            <img src="{{tag.image}}" alt="" />
          </ion-card>
          <h2>{{tag.title}}</h2>
        </ion-slide>
      </ion-slides>
slideOptions = {
    loop: false,
    initialSlide: 0,
    slidesPerView: 1,
    centeredSlides: false,
    spaceBetween: 10
  };
@liamdebeasi

This comment has been minimized.

Copy link
Member Author

commented Apr 5, 2019

Hi there,

If you have found a bug in ion-slides in the latest version of Ionic, please open a new bug report here: https://github.com/ionic-team/ionic/issues/new?assignees=&labels=&template=bug_report.md&title=bug%3A+

This PR is to expose interfaces for custom animations and does not fix any other bugs.

Thanks!

@liamdebeasi liamdebeasi moved this from In progress 🤺 to Needs review 🤔 in Ionic Core Apr 5, 2019

@liamdebeasi liamdebeasi requested a review from brandyscarney Apr 11, 2019

@brandyscarney

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

I'm a bit confused by the usage:

  1. Do I have to pass effect now? What's the point of passing slide if the default is slide and flip if I still have to define the effect?
  2. Where can I find more animation examples? I know there are links to the framework tests but there are some problems with this
    • what if we get rid of those tests?
    • what if we want something that isn't listed?

I think it would be better to just show the usage for them instead of having it in a test and then link to where it came from (probably swiper docs?)

@liamdebeasi

This comment has been minimized.

Copy link
Member Author

commented Apr 11, 2019

  1. Yea for some of them there is corresponding CSS so you'll need to pass in the effect name. I need to double check about "slide" though.

  2. Good question - still trying to figure out a good place for these. We provided the remaining animations that are bundled by default in Swiper, but beyond that that's on the user to come up with. For the actual code, I got it from the Swiper repo so unless we host the code somewhere, we're just going to be pointing users to some github repo.

@liamdebeasi

This comment has been minimized.

Copy link
Member Author

commented Apr 11, 2019

Took a look again and seems like this was due to us always explicitly saying to use the slide effect internally. I updated it so you don't need to provide any effect value and swiper will just default to slide if the animation handlers are not overriden. The effect value is only useful if the animations are built in.

@brandyscarney

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

Friendly reminder, don't forget about this comment: #16616 (comment)

liamdebeasi added some commits Apr 12, 2019

Show resolved Hide resolved core/src/components/slides/readme.md Outdated

brandyscarney and others added some commits Apr 16, 2019

Update core/src/components/slides/readme.md
Co-Authored-By: liamdebeasi <liamdebeasi@users.noreply.github.com>

@liamdebeasi liamdebeasi merged commit 4474974 into master Apr 16, 2019

2 checks passed

build Workflow: build
Details
screenshot Screenshot
Details

Ionic Core automation moved this from Needs review 🤔 to Done 🎉 Apr 16, 2019

@liamdebeasi liamdebeasi deleted the slides-custom-animations branch Apr 16, 2019

@CFT-Chris

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

I'm a bit late to this party, but I have seen the changes on the docs.

Has anyone looked at my suggestion to just add a swiperlib attribute so that all swiper plugins can be exposed to "advanced" users looking for them? I think the current stream of changes to ion-slides is akin to running on a treadmill: every new feature someone wants from the Slider library won't be readily available to someone using ion-slides until another PR is pushed.

Me personally, I need the "navigation" arrows, and I have a branch ready that proposes the required changes to ion-slides to make them work. I've seen others looking for "parallax" effects, virtual slides, etc. and of course this PR addressed those looking for the flip/coverflow/fade/cube effects.

I'm not sure if it's a great developer experience to have devs copy/paste chunks of swiper code into their slides options to make them work.

If we allow devs to import their own Swiper library (custom built with the features they want) and have ion-slides consume that library per my suggestion here, then all Swiper options would be available to them with all the added Ionic benefits that come with ion-slides.

Thoughts?

Kiku-git added a commit to Kiku-git/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
You can’t perform that action at this time.