Skip to content

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

Merged
merged 11 commits into from
Dec 19, 2018
Merged

feat(carousel): Count slides by custom tag name #322

merged 11 commits into from
Dec 19, 2018

Conversation

ashtonmeuser
Copy link
Contributor

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

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.
  • I have included a vue-play example (if this is a new feature)

AM and others added 5 commits November 23, 2018 15:30
…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
@csmoakpax8
Copy link
Contributor

@ashtonmeuser This would be great!
@quinnlangille Do you have any issues with merging this?

@ashtonmeuser
Copy link
Contributor Author

@quinnlangille Bump. Is there anything you'd like me to add to encourage this to be merged?

@quinnlangille
Copy link
Member

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 :octocat:

@quinnlangille quinnlangille changed the base branch from master to v0.17.0 December 18, 2018 16:19
@quinnlangille
Copy link
Member

^ just resolved some small merge conflicts in the build files.

@coveralls
Copy link

coveralls commented Dec 18, 2018

Coverage Status

Coverage increased (+0.3%) to 67.593% when pulling dea03e9 on ashtonmeuser:feat/slide-component-extension-support into 7d5cb5c on SSENSE:v0.17.0.

@quinnlangille
Copy link
Member

Ah dang, looks like the branch is going to need a rebase with 0.17.0. Would you mind rebasing and pushing back up when you get a sec? If you do that, feel free to ignore the merge I made and fix the conflicts locally :octocat:

@ashtonmeuser
Copy link
Contributor Author

Will do. I must have fallen behind.

@quinnlangille
Copy link
Member

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

@ashtonmeuser
Copy link
Contributor Author

@quinnlangille Turned out to be the migration to Vue Test Utils. Fixed.

@quinnlangille
Copy link
Member

Ahhh perfect @ashtonmeuser, thanks for branch update 👍 Will merge now :octocat:

@quinnlangille quinnlangille merged commit a895b28 into SSENSE:v0.17.0 Dec 19, 2018
quinnlangille pushed a commit that referenced this pull request Dec 19, 2018
* 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
josephting added a commit to josephting/vue-carousel that referenced this pull request Dec 20, 2018
* tags adjustments from SSENSE#349

* pre jest-serializer-vue test from SSENSE#322
quinnlangille pushed a commit that referenced this pull request Dec 27, 2018
* 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
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.

5 participants