Bug fix for slider swipe

[rolandschuetz]

If there are just two slides the setup function clones the nodes on line 50. When the browser then is resized the updateMenu function is triggered which thinks there should be 4 dots, not two dots and therefore produces an error.

[aschempp]

I remember a similar issue was fixed some time ago, but I could not find the ticket...

[rolandschuetz]

Haha, then it obviously wasn't merged yet. But merging this should be pretty straight forward.

I just wasn't sure how you are normally minifying, so I used the YUI compressor.

Cheers,
Roland

[rolandschuetz]

I just tested against IE 8-11 and found another bug to be fixed for IE8, see new commit.

[leofeyer]

There aren't any comments in my markup. How do I reproduce?

[rolandschuetz]

The pull request describes exactly the reason for the first commit. If there are exactly two slides the length property claims 4 and therefore updating the menu results in an js error.

The second commit has following comment exactly explaining the problem. The result of this problem is that in IE8 the comments are also counted and slides*2 navigation points are created.

// Internet explorer lower than 9 also lists comment nodes in children
// therefore count instead of using slides.length

[leofeyer]

It is generally not a good idea to mix two issues in the same pull request (or ticket). It will make things difficult if e.g. each issue needs to be assigned to a different milestone or if both issues cannot be resolved at the same time.

Regarding issue 1 (slide.length): I will look into it.

Regarding issue 2 (IE8 comments): I still don't know what you mean. What comments? There aren't any in my markup. How do I reproduce this in the online demo?

[rolandschuetz]

Oh, can I make two different pull requests from one repository?

About the 2. issue:
It's about html comments. If I add for example a mod_newslist element there is a indexer::stop comment added. IE 8 counts this comment dom nodes and therefore creates too many slider menu points.

[discordier]

@rolandschuetz To make different pull requests, you create separate branches in your repository.
It is pretty common to name them after the issue they address.

It depends on the GIT interface you use but in plain command line the usual workflow goes like this:

# clone your own fork
git clone [email protected]:rolandschuetz/core.git
# add the upstream repository to the fork.
git add remote upstream [email protected]:contao/core.git
# checkout the correct maintenance branch you want to base upon
# (ie. master, develop, hotfix/3.2.15).
git checkout upstream/hotfix/3.2.15
git checkout -b fix-slider-swipe-issue
# now hack your code and commit
git commit -m "fix issue #123456789"
# push your changes to github
git push origin fix-slider-swipe-issue

# find another issue.
git checkout upstream/hotfix/3.2.15
# create another branch
git checkout -b fix-ie-comment-bug
# now hack your code and commit
git commit -m "fix issue #99999999"
# push your changes to github
git push origin fix-ie-comment-bug

That way you now have two different branches in your fork which you can hand in as pull request and even alter separately if needed without mixing the stuff.

If you understand German, the following might be of use to you: http://de.contaowiki.org/Core_Forking_-_best_practice

[rolandschuetz]

Ah, vielen Dank!

[leofeyer]

The issue with having exactly two slides has indeed been fixed already.

The other issue is valid, however your patch does not fix it. It does reduce the number of dots in the dot menu to two, but you can still navigate all four tabs with the "next" button.

[leofeyer]

Fixed in contao-components/swipe@d43e48c.

[rolandschuetz]

Oh, possible that I missed it that. Thanks for fixing it.