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

Added Array type to navigateTo prop type check #366

Merged
merged 1 commit into from
Feb 19, 2019
Merged

Added Array type to navigateTo prop type check #366

merged 1 commit into from
Feb 19, 2019

Conversation

rico-ocepek
Copy link
Contributor

@rico-ocepek rico-ocepek commented Feb 6, 2019

Description

Changed the property type of the carousel's navigateTo property from Number only to [Number, Array].
Also updated readme.md to describe the new behaviour.
Updated the "NavigateTo slides" Vue play example to include an example of the new API usage.

Motivation and Context

As the pending fix from https://github.com/SSENSE/vue-carousel/issues/257 should finally be done I decided to help you out a bit.

How Has This Been Tested?

As the feature was already implemented and only the property type was wrong further tests should not be required.
However I did test both cases of the navigateTo property (animated and non animated) and couldn't spot any errors.
As I said I also included an example in the Vue play file for testing purposes.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

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)

Updated readme.md; Added example usage to Vue play file
@coveralls
Copy link

Coverage Status

Coverage remained the same at 67.173% when pulling 56cee00 on rico-ocepek:feature/fixNavigateToProp into 3a5c1c6 on SSENSE:v0.18.0.

@SSENSE SSENSE deleted a comment from rico-ocepek Feb 8, 2019
@quinnlangille
Copy link
Member

Hey @rico-ocepek, thanks for this! I'll test and merge in this afternoon 🚀

@rico-ocepek
Copy link
Contributor Author

Hi @quinnlangille, sorry to bother you again - do we have any progress here? Would love to integrate this feature in a client project without anyone asking why there is an error message 😅

@quinnlangille quinnlangille merged commit e13e339 into SSENSE:v0.18.0 Feb 19, 2019
@quinnlangille
Copy link
Member

Hey @rico-ocepek sorry for the delay! I've merged and published as an alpha tag since you needed it quickly - you can find it with yarn install vue-carousel@0.18.0-alpha 🚀

@rico-ocepek
Copy link
Contributor Author

No worries @quinnlangille, thank you for your help and your great work here! 👍🏻

quinnlangille pushed a commit that referenced this pull request Mar 18, 2019
Updated readme.md; Added example usage to Vue play file
quinnlangille pushed a commit that referenced this pull request Mar 21, 2019
Updated readme.md; Added example usage to Vue play file
quinnlangille pushed a commit that referenced this pull request Mar 21, 2019
Updated readme.md; Added example usage to Vue play file
quinnlangille pushed a commit that referenced this pull request Mar 21, 2019
Updated readme.md; Added example usage to Vue play file
quinnlangille pushed a commit that referenced this pull request Mar 21, 2019
Updated readme.md; Added example usage to Vue play file
quinnlangille pushed a commit that referenced this pull request Mar 21, 2019
Updated readme.md; Added example usage to Vue play file
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.

4 participants