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

ion-slides use zoom="true" report errors #12861

Closed
cuidong626 opened this issue Sep 12, 2017 · 9 comments · Fixed by #12931
Closed

ion-slides use zoom="true" report errors #12861

cuidong626 opened this issue Sep 12, 2017 · 9 comments · Fixed by #12931
Assignees
Labels
needs: reply the issue needs a response from the user

Comments

@cuidong626
Copy link

cuidong626 commented Sep 12, 2017

Resources:
Before submitting an issue, please consult our troubleshooting guide (http://ionicframework.com/docs/troubleshooting/) and developer resources (http://ionicframework.com/docs/developer-resources/)

Ionic version: (check one with "x")
[ ] 1.x (For Ionic 1.x issues, please use https://github.com/ionic-team/ionic-v1)
[ ] 2.x
[x] 3.x

I'm submitting a ... (check one with "x")
[x] bug report
[ ] feature request
[ ] support request => Please do not submit support requests here, use one of these channels: https://forum.ionicframework.com/ or http://ionicworldwide.herokuapp.com/

Current behavior:
<ion-slides zoom="true"> use zoom report errors.
The swiper official website says that if zoom is enabled, you need to add class= "swiper-zoom-container". Sorry, my English is weak

Expected behavior:

Steps to reproduce:

Related code:

insert any relevant code here

Other information:
[]

Ionic info: (run ionic info from a terminal/cmd prompt and paste output below):

cli packages: (C:\Users\Administrator\AppData\Roaming\npm\node_modules)

    @ionic/cli-utils  : 1.10.2
    ionic (Ionic CLI) : 3.10.3

local packages:

    @ionic/app-scripts : 2.1.3
    Ionic Framework    : ionic-angular 3.6.1

System:

    Node : v6.11.0
    npm  : 3.10.10
    OS   : Windows 7
@kensodemann
Copy link
Member

Hello! Thank you for opening an issue with us! Would you be able to provide a sample application via GitHub that demonstrates the issue you are having? Thanks for using Ionic!

@gabfiocchi
Copy link

gabfiocchi commented Sep 13, 2017

Same problem here, if you are try to enable zoom on ion-slides components and have another element of img, svg, ion-img or canvas, swiper-gesture crash. But if you have one of this elements, the zoom not working.

Try next code, and you can check pinch zoom-in or zoom-out:

<ion-slides zoom>
    <ion-slide>
        <img src="assets/img/demo.jpg">
    </ion-slide>
  </ion-slides>

Can you fix? Thanks.

@pablo-gonzalez
Copy link

Hi @kensodemann

It is very easy to reproduce this issue. Install the latest version of ionic:
npm install -g ionic@latest
Then
ionic start slideTest blank
In home.html add this code:

 <ion-slides zoom="true">
    <ion-slide>
      <h1>Slide 1</h1>
    </ion-slide>
    <ion-slide>
      <h1>Slide 2</h1>
    </ion-slide>
    <ion-slide>
      <h1>Slide 3</h1>
    </ion-slide>
  </ion-slides>

Ionic serve
If you try to zoom in your device you will get the error.
In desktop you have to make double click inside content slide.

Tested using Chrome for Android and Desktop.

captura de tela de 2017-09-13 19-49-50


pablo@pablo-Inspiron-7520:~/projects/slideTest$ ionic info

cli packages: (/usr/lib/node_modules)

    @ionic/cli-utils  : 1.10.2
    ionic (Ionic CLI) : 3.10.3

local packages:

    @ionic/app-scripts : 2.1.4
    Ionic Framework    : ionic-angular 3.6.1

System:

    Node : v7.10.0
    npm  : 3.10.10 
    OS   : Linux 4.4

@kensodemann kensodemann self-assigned this Sep 14, 2017
@kensodemann
Copy link
Member

Thank you. I have used your code to duplicate this issue.

@kensodemann
Copy link
Member

@pablo-gonzalez - your sample code has zoom true but no images. The swiper does not support that. Do you have a use case for not having images on these pages? (not that zooming would work, but just wanted to check)

@gabfiocchi - if you add a zoom container per the swiper docs, then your example works just fine.

    <ion-slide>
      <div class="swiper-zoom-container">
        <img src="assets/img/demo.jpg">
      </div>
    </ion-slide>

@kensodemann kensodemann added the needs: reply the issue needs a response from the user label Sep 19, 2017
@pablo-gonzalez
Copy link

Hi @kensodemann

I was planning to use zoom with an image but I found the issue by accident while I was testing my app on mobile and desktop.
My idea is to put one image and some text in the slide.

According to your last post you need to add a zoom containter to make it work with the image.

I could not find any reference to that in the Ionic documentation:
https://ionicframework.com/docs/api/components/slides/Slides/

Input Properties

zoom | boolean | If true, enables zooming functionality.

I think you should add some clarification about adding the zoom containter with class="swiper-zoom-container" because it is required by the component to work properly when you set zoom="true".

If you implemented safe-guards when trying to make zoom without image it seems good to me.
How can I test that new version?

Thanks

@kensodemann
Copy link
Member

If you implemented safe guards when trying to make zoom without image it seems good to me.

That is all experimental code. I suppose you could fork that branch if you want, but I wouldn't. I doubt it will get merged given the amount of rework being done in this area in v4, which will use the swiper more directly.

Plus, the problem isn't really the lack of safeguards. Everything works fine with the existing code if you follow the swiper API docs, and one could argue that at least if you do things wrong it crashes where-as with my code in place it would just fail to zoom.

You can do an image and text in each slide and it will still work just fine. As an example

    <ion-slide>
      <div class="swiper-zoom-container">
        <img src="assets/img/demo.jpg">
      </div>
      <ion-label>I am a test app</ion-label>
    </ion-slide>

So just use the current version (3.6.1) and make sure you have an image and a wrapper, and you will be fine.

Updating the documentation to include that you need to have an image on each slide and that each slide should be contained with a swiper-zoom-container is a good idea.

@pablo-gonzalez
Copy link

pablo-gonzalez commented Sep 19, 2017

What happened to me is that I just put the previous code taken from Ionic documentation for testing and zoom="true" and once in the desktop browser I got the error doing double click on the slide content.

Updating the documentation would be great to avoid bad use of the component. I was not aware that zoom only worked with images and I needed to add a container and a class as additional steps.

Thank you for your support.

Best regards,
Pablo

@ionitron-bot
Copy link

ionitron-bot bot commented Sep 2, 2018

Thanks for the issue! This issue is being locked to prevent comments that are not relevant to the original issue. If this is still an issue with the latest version of Ionic, please create a new issue and ensure the template is fully filled out.

@ionitron-bot ionitron-bot bot locked and limited conversation to collaborators Sep 2, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs: reply the issue needs a response from the user
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants