-
Notifications
You must be signed in to change notification settings - Fork 504
feat(carousel): Count slides by custom tag name #322
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(carousel): Count slides by custom tag name #322
Conversation
…323) * fix(carousel): Flush the slides left when they don't fill the width (#323) When there are too few slides to fill the row, with scrollPerPage, the slides were flushed right instead of left. By bounding maxOffest to a minimum of 0, the slides are aligned on the left-hand-side Fix #323 * style(play): Use capitals in tests for consistency
@ashtonmeuser This would be great! |
@quinnlangille Bump. Is there anything you'd like me to add to encourage this to be merged? |
Hey @ashtonmeuser, sorry for the delay on this one! We've been crazy busy at the office with year end wrap-up and I've been delinquent on PR reviews. This is a brilliant feature! We had something similar planned internally, but this will meet the needs for sure. I'll merge this into our working branch for next release, it should be out late this week or early next! 🚀 Thanks for the contribution, and again sorry for the delay on review! We'll be more on-top of things going forward |
^ just resolved some small merge conflicts in the build files. |
Ah dang, looks like the branch is going to need a rebase with |
Will do. I must have fallen behind. |
Yeah we merged a few other PRs into the working branch since, so will just need to catch up with those. It's a few commits ahead of master |
@quinnlangille Turned out to be the migration to Vue Test Utils. Fixed. |
Ahhh perfect @ashtonmeuser, thanks for branch update 👍 Will merge now |
* feat(carousel): Count slides by custom tag name * fix(carousel): flush the slides left when they don't fill the width (#323) * fix(carousel): Flush the slides left when they don't fill the width (#323) When there are too few slides to fill the row, with scrollPerPage, the slides were flushed right instead of left. By bounding maxOffest to a minimum of 0, the slides are aligned on the left-hand-side Fix #323 * style(play): Use capitals in tests for consistency * v0.16.1 * chore: build * Rebuild * Use Vue Test Utils, rebuild * Discard change to package version
* tags adjustments from SSENSE#349 * pre jest-serializer-vue test from SSENSE#322
* feat(carousel): Count slides by custom tag name * fix(carousel): flush the slides left when they don't fill the width (#323) * fix(carousel): Flush the slides left when they don't fill the width (#323) When there are too few slides to fill the row, with scrollPerPage, the slides were flushed right instead of left. By bounding maxOffest to a minimum of 0, the slides are aligned on the left-hand-side Fix #323 * style(play): Use capitals in tests for consistency * v0.16.1 * chore: build * Rebuild * Use Vue Test Utils, rebuild * Discard change to package version
Enable components to extend slide. Carousel considers child components slides if component name matches
tagName
.Description
Add
tagName
property to carousel. Defaults to "slide", which is currently used. Counts child slide components by matching whole tag name (regex^vue-component-\\d+-${this.tagName}$
used). Added test.Motivation and Context
When wrapping slides, the wrapping components tag name (not necessarily "slide") prevents it from being considered a slide within a carousel. This results in 0 extended slides showing in carousel. This circumvents requiring extending components to have the name "slide" (of particular annoyance if using ESLint's default preference of PascalCase component names).
If the carousel is intended to have similarly structured slides, there is a lot of boilerplate required. This change allows structuring a component that wraps slide. A simple title or body property could then be supplied to each extended slide.
How Has This Been Tested?
Added test to carousel.spec.js. Test adds three slides to a carousel, two of which are extend slide. The carousel is set to match the tag names of the extended slides. It should only count extended slides.
Types of changes
Checklist: