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 peeking with disabled state #102

Merged
merged 2 commits into from
Sep 28, 2022
Merged

Conversation

weotch
Copy link
Member

@weotch weotch commented Sep 23, 2022

Fixes #95

@codesandbox
Copy link

codesandbox bot commented Sep 23, 2022

CodeSandbox logoCodeSandbox logo  Open in CodeSandbox Web Editor | VS Code | VS Code Insiders

@netlify
Copy link

netlify bot commented Sep 23, 2022

Deploy Preview for vue-ssr-carousel ready!

Name Link
🔨 Latest commit dbdff2c
🔍 Latest deploy log https://app.netlify.com/sites/vue-ssr-carousel/deploys/632e23a133e24b0008123c1e
😎 Deploy Preview https://deploy-preview-102--vue-ssr-carousel.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@weotch
Copy link
Member Author

weotch commented Sep 23, 2022

@fgrasu can you let me know if this fixes your issue from #95? You can use this branch if you change your version constraint to vue-ssr-carousel: BKWLD/vue-ssr-carousel#dbdff2cc

@cypress
Copy link

cypress bot commented Sep 23, 2022



Test summary

14 0 0 0


Run details

Project vue-ssr-carousel
Status Passed
Commit dbdff2c
Started Sep 23, 2022 9:23 PM
Ended Sep 23, 2022 9:25 PM
Duration 02:16 💡
OS Linux Ubuntu - 20.04
Browser Electron 102

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@fgrasu
Copy link

fgrasu commented Sep 23, 2022

hi @weotch, thanks for working on this issue. it's a partially fix, if you remove the :slides-per-pages="4" from default props or if you have another slidesPerPage in responsive, something like this:
image
then the carousel is still broken.
btw, I used your branch as a version like you said but I didn't get the changes, I applied the style changes manually. maybe I did something wrong idk.

@weotch
Copy link
Member Author

weotch commented Sep 24, 2022

I couldn't reproduce the issue you're mentioning with those props.

It looks wonky, but that's just because it's trying to cram 10 in that space.

image
image

@fgrasu
Copy link

fgrasu commented Sep 24, 2022

Oh.. I was thinking that, if the slidesPerPage value is higher that the carousel length, it should take the carousel length value instead. that way will not look wonky, it will display 2 slides to fill the viewport (without peeking, if any is set, because doesn't make sense in this case).
In your code above, can you try to delete the :slides-per-pages="4" from default props? there are no slides displayed. Can you reproduce that?

@weotch
Copy link
Member Author

weotch commented Sep 24, 2022

Oh.. I was thinking that, if the slidesPerPage value is higher that the carousel length, it should take the carousel length value instead. that way will not look wonky, it will display 2 slides to fill the viewport (without peeking, if any is set, because doesn't make sense in this case).

I think with same layouts, you'd want the width of the slides to be consistent, even if there weren't enough to fill the carousel width. Like maybe you don't want to scale up a slide to 100% if there was only one. If you are rendering your slides with a v-for, I think you could accomplish what you're after with:

<ssr-carousel :slides-per-page='Math.min(10, slides.length)'>
  <slide v-for='for slide in slides' />
</ssr-carousel>

In your code above, can you try to delete the :slides-per-pages="4" from default props? there are no slides displayed. Can you reproduce that?

Still seems fine to me, unless codesandbox isn't updating like it should.

image

@fgrasu
Copy link

fgrasu commented Sep 26, 2022

It seems like if you set peek="0" with 0 as a string you might have some issues. that's why you couldn't reproduce my scenario. :peek="0" will work just fine.
Thanks for patience. I'm good with your PR solution

@weotch weotch merged commit f86395a into main Sep 28, 2022
@weotch weotch deleted the fix-peeking-with-disabled-state branch September 28, 2022 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue with disabling behavior when a peek and loop are specified
2 participants