-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
Accordion: correct height calculated when closed #1536
Conversation
7795794
to
5d225dd
Compare
I have no idea why that unit test is failing. All the tests pass when I run them on my machine, so I don't know how I can fix it. |
Can we get a test for this? |
@@ -361,7 +361,10 @@ return $.widget( "ui.accordion", { | |||
maxHeight = 0; | |||
this.headers.next() | |||
.each( function() { | |||
var display = $( this ).css( "display" ); | |||
$( this ).css( "display", "block" ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just use $( this ).show()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, more explicitly, follow this logic:
Lines 634 to 642 in adcc8ef
// Need to show the dialog to get the actual offset in the position plugin | |
var isVisible = this.uiDialog.is( ":visible" ); | |
if ( !isVisible ) { | |
this.uiDialog.show(); | |
} | |
this.uiDialog.position( this.options.position ); | |
if ( !isVisible ) { | |
this.uiDialog.hide(); | |
} |
5d225dd
to
7e4d6bf
Compare
I've changed it to use is( ":visible" ) and show() / hide(), but that unit test is still failing while it passes if I run it on my machine. It seems like calling show() and hide() causes some change in the element that assert.domEqual detects, but not when I run the test locally. |
I'm not sure what's going on with that failure either. Can you go ahead with the rest of the changes? I'll try to dig into the Travis failure. |
I've added a unit test. heightStyle "fill" uses height() - innerHeight() which just measures padding, and is not effected by the visibility of the element, so no changes should be necessary. I tested this in Chrome, Firefox, and IE. |
@@ -10,6 +10,18 @@ var equalHeight = testHelper.equalHeight, | |||
|
|||
module( "accordion: methods", setupTeardown() ); | |||
|
|||
test( "collapsedHeight", function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're testing a set of options, not a method, so this is in the wrong file and the test title doesn't really make sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add // http://bugs.jqueryui.com/ticket/11938
above the test.
The changes and test look good. Just a minor note about the placement of the test. If you'd prefer, I can make that change myself when I land this. Otherwise, I'll just wait to hear back from you. Thanks. |
Also, we still have no idea why that test is failing in Travis, but we'll keep digging. |
Can you rebase on master and see if that helps at all with Travis? It's a clean rebase, I tested locally. |
Fixes #11938
Fixes #11938
white space correction
Ref #11938
5fa27f3
to
46e065e
Compare
I did a rebase on master which did not fix the Travis issue. I also moved and renamed the unit test. |
I'm still trying to figure out what's causing the test to fail. So far I've tracked it down to this difference: < "WebkitTransformOrigin": "500px 22px",
> "WebkitTransformOrigin": "492px 22px",
< "WebkitPerspectiveOrigin": "500px 22px",
> "WebkitPerspectiveOrigin": "492px 22px",
< "width": "1000px",
> "width": "984px", |
Fixes #11938 Closes jquerygh-1536
Fixes #11938 Closes jquerygh-1536
Fixes #11938 Closes jquerygh-1536
Fixes #11938 Closes jquerygh-1536
Fixes #11938 Closes jquerygh-1536
Fixes #11938 Closes jquerygh-1536
Fixes #11938 Closes jquerygh-1536
Fixes #11938 Closes jquerygh-1536
Fixes #11938 Closes jquerygh-1536
Fixes #11938 Closes jquerygh-1536
It's been almost a year, so you've probably forgotten about this by now. But I have good news! We just upgraded to Phantom 2 and the tests now pass on Travis. |
Fixes #11938