Skip to content

Fix the carousel option#47

Merged
peduarte merged 5 commits into
peduarte:masterfrom
jacobmllr95:store-direction
Sep 16, 2015
Merged

Fix the carousel option#47
peduarte merged 5 commits into
peduarte:masterfrom
jacobmllr95:store-direction

Conversation

@jacobmllr95

@jacobmllr95 jacobmllr95 commented Sep 15, 2015

Copy link
Copy Markdown
Contributor
  • Detect the correct direction in the goTo function to set the
    correct classes.
    • To detect a forwards direction we need to check if the index is
      higher than the currentItemIndex or if the index is 0 an the
      currentItemIndex is the index of the last item (go from the last item
      to the first).
    • To detect a backwards direction we need to check if the index
      is lower than the currentItemIndex or if the index is the index
      of the last item and the currentItemIndex is 0 (go from the first
      item to the last).
  • Fix the initial button state when the carousel option is disabled.

@jacobmllr95
Merge pull request #1 from peduarte/master
c754226 
Sync latest changes
@jacobmllr95
Improve addClass and removeClass functions
1bc3fa4 
This changes make sure that the className doesn’t contain any trailing
spaces.
@jacobmllr95
Fix the carousel option
f9572f3 
* Detect the correct direction in the `goTo` function  to set the
correct classes.
    * To detect a forwards direction we need to check if the `index` is
higher than the `currentItemIndex` **or** if the `index` is `0` an the
`currentItemIndex` is the index of the last item (go from the last item
to the first).
    * To detect a backwards direction we need to check if the `index`
is lower than the `currentItemIndex` **or** if the `index` is the index
of the last item and the `currentItemIndex` is `0` (go from the first
item to the last).
* Fix the initial button state when the `carousel` option is disabled
@jacobmllr95 jacobmllr95 mentioned this pull request Sep 15, 2015
Closed
@peduarte

peduarte commented Sep 15, 2015

Copy link
Copy Markdown
Owner

Thanks @jackmu95 will take a proper look at this later on! 👏

@jacobmllr95

jacobmllr95 commented Sep 15, 2015

Copy link
Copy Markdown
Contributor Author

No problem :)

I forgot to create a separate branch for my fist pull request (#46) so this is also included in this PR. I hope it's OK... But while testing anything was fine 👍

@peduarte

peduarte commented Sep 15, 2015

Copy link
Copy Markdown
Owner

So this PR includes the changes you made in #46? Can I close #46 then?

@jacobmllr95

jacobmllr95 commented Sep 15, 2015

Copy link
Copy Markdown
Contributor Author

Yes it does. And you can close #46.

@peduarte peduarte mentioned this pull request Sep 15, 2015
Closed
@peduarte

peduarte commented Sep 15, 2015

Copy link
Copy Markdown
Owner

This will need some tests written to make sure it's all working properly.

For example (in pseudo code)

if click on [next] 
when current item is [last]
[nextItem] to have class [Wallop-item--showNext]
...test other classes...

Similar tests for backwards direction

If you are happy writing the tests, I can then double check everything is working and merge this PR.

Otherwise, I will write tests later

@jacobmllr95

jacobmllr95 commented Sep 15, 2015

Copy link
Copy Markdown
Contributor Author

I will write the tests and update the PR 👍

@jacobmllr95

jacobmllr95 commented Sep 15, 2015

Copy link
Copy Markdown
Contributor Author

I added a test for the button state on initialization when the carousel option is disabled and updated the test for the carousel.

To test the button state I made some changes to the createSlider function. I does now add the previous and next button to the wallop slider.

peduarte
peduarte reviewed Sep 15, 2015
View reviewed changes
Comment thread js/Wallop.js Outdated

@peduarte peduarte Sep 15, 2015

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the use case of doing this?

@jacobmllr95 jacobmllr95 Sep 15, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the carousel option is disabled, the previous button gets disabled when you are on the first item and the next button gets disabled when you are on the last item. When the wallop slider is initialized and the start item is the first or the last one, the depending button isn't disabled (see http://codepen.io/anon/pen/zvrWpp).

jacobmllr95
jacobmllr95 reviewed Sep 15, 2015
View reviewed changes
Comment thread js/Wallop.js

@jacobmllr95 jacobmllr95 Sep 15, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is needed to set the correct button state on initialization when the carousel option is disabled. This was not the case before (see http://codepen.io/anon/pen/zvrWpp). To get the correct state for the previous button, you need to click next and the previous.

@peduarte peduarte Sep 15, 2015

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh yes, well spotted! 👍

@jacobmllr95 jacobmllr95 Sep 15, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks ;)

peduarte pushed a commit that referenced this pull request Sep 16, 2015
Merge pull request #47 from jackmu95/store-direction
9be5c44 
Fix the `carousel` option
@peduarte peduarte merged commit 9be5c44 into peduarte:master Sep 16, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jacobmllr95 @peduarte