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

Fix bug #16980 #17735

Closed
wants to merge 2 commits into from
Closed

Fix bug #16980 #17735

wants to merge 2 commits into from

Conversation

phmasek
Copy link

@phmasek phmasek commented Sep 29, 2015

Added a check if there are more than one tab active, make the one with highest index inactive.

@twbs-savage
Copy link

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 7c089f3
Build details: https://travis-ci.org/twbs-savage/bootstrap/builds/82766255

(Please note that this is a fully automated comment.)

@phmasek
Copy link
Author

phmasek commented Sep 29, 2015

Seems like an unsuccessful test in something unrelated to my fix. In Mac OS X, Safari.

Tested on local computer (Mac OS X, Safari) with full success.

image

@cvrebert
Copy link
Collaborator

@twbs-savage Retry

@twbs-savage
Copy link

Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: 7c089f3
Build details: https://travis-ci.org/twbs-savage/bootstrap/builds/82780525

(Please note that this is a fully automated comment.)

@cvrebert
Copy link
Collaborator

@Pletron Please add a relevant unit test to the test suite. See https://github.com/twbs/bootstrap/tree/master/js/tests for details.

@phmasek
Copy link
Author

phmasek commented Sep 29, 2015

@cvrebert There is an issue with creating a unit test for this particular bug fix. The fix addresses an issue that can only be replicated when running a .click() event. However, running such an event is not possible in the test suite (see image). The workaround is to run $('#tab-button').bootstrapTab('show'), but this does not replicate the issue.

click is not a function

I can write the unit test with the workaround, but it is completely obsolete for this purpose. What do you suggest?

@cvrebert
Copy link
Collaborator

Use .trigger('click') instead of .click()
Example:

$tabs.find('li:last a').trigger('click')

@twbs-savage
Copy link

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 6386cf2f14853b3231b4540996b05d6bbdbb3cde
Build details: https://travis-ci.org/twbs-savage/bootstrap/builds/82790083

(Please note that this is a fully automated comment.)

@cvrebert
Copy link
Collaborator

Hmm, actually, sounds like this involves transitions, in which case a unit test won't work because the test suite runs with transitions disabled. Guess this will need to be a manual visual test (https://github.com/twbs/bootstrap/blob/master/js/tests/visual/tab.html ) instead.

@phmasek
Copy link
Author

phmasek commented Sep 29, 2015

@cvrebert Yeah, tested it there with the existing code through the console and it works fine. How would you suggest that I write the test there?

To test i ran: $('#myTab1 > :eq(1) a').click();$('#myTab1 > :eq(0) a').click(); in the console. This triggers a quick switch between the tabs.

@twbs-savage
Copy link

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 7c089f3
Build details: https://travis-ci.org/twbs-savage/bootstrap/builds/82793802

(Please note that this is a fully automated comment.)

@cvrebert
Copy link
Collaborator

Add another section to /js/tests/visual/tab.html with a set of tabs and a button with a click handler that runs $('#myTab1 > :eq(1) a').click();$('#myTab1 > :eq(0) a').click();.

@twbs-savage
Copy link

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 8c1bfd7
Build details: https://travis-ci.org/twbs-savage/bootstrap/builds/82800632

(Please note that this is a fully automated comment.)

@cvrebert
Copy link
Collaborator

Sauce failure is due to an unrelated Sauce error.

@phmasek
Copy link
Author

phmasek commented Sep 29, 2015

@cvrebert Yes, seems like issues with Sauce. Anything more I can do here?

@cvrebert
Copy link
Collaborator

No, just needs code review now.
CC: @fat

@cvrebert cvrebert modified the milestone: v3.3.6 Oct 13, 2015
@cvrebert cvrebert closed this in 03dbebb Oct 13, 2015
@cvrebert
Copy link
Collaborator

Sorry, got the PR numbers mixed up.

@cvrebert cvrebert reopened this Oct 13, 2015
@mdo
Copy link
Member

mdo commented Sep 5, 2016

Bootstrap 3 is no longer being officially developed or supported.

All work has moved onto our next major release, v4. As such, this issue or pull request is being closed as a "won't fix." For additional help and support, we recommend utilizing our community resources. Thanks for your understanding, and see you on the other side of v4!

<3,
@mdo and team

@mdo mdo closed this Sep 5, 2016
chiraggmodi pushed a commit to chiraggmodi/bootstrap that referenced this pull request Apr 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants