Slide panel refactor tests #5412

Closed
wants to merge 8 commits into
from

Conversation

Projects
None yet
6 participants
Contributor

zachleat commented Jan 2, 2013

  • Q-unit Tests for panel.
  • Adds immediate on panel open/close methods.
  • Fix for Firefox issue where the transitionend event wasn't firing on open (no animation).

zachleat referenced this pull request Jan 2, 2013

Closed

Responsive sliding panels #5163

Member

jaspermdegroot commented Jan 3, 2013

@zachleat

Let me know if you want me to merge this in the branch or if you want @scottjehl / @jlembeck to have a look at it first.

Contributor

zachleat commented Jan 3, 2013

I think @scottjehl is planning on reviewing it when he gets a chance, either tomorrow or early next week.

Thanks!

Owner

arschmitz commented Jan 3, 2013

@zachleat @toddparker had asked me to look at some panel issues i noticed last week and it looks like the changes i made are going to cause some merge conflicts. I already implemented immediate for open and close also as it was the solution to some issues I noticed. If you want i can go ahead and take care of your changes to panel.js they are very simple. and the changes in panel js are the conflict. ( i didn't look at tests at all ).

Contributor

zachleat commented Jan 3, 2013

@arschmitz I'd recommend sticking with your immediate implementation, since I didn't implement to solve any issues. The other JavaScript changes are whitespace/code-cleanup things. The one CSS change is a bit more important (it solves the FF issue mentioned above).

Owner

arschmitz commented Jan 3, 2013

@zachleat our implementations are the same :) ... ill take care of your css and panel changes and leave the tests for @scottjehl to look at.

@johnbender johnbender and 1 other commented on an outdated diff Jan 3, 2013

tests/unit/panel/panel_core.js
- panel5 = $( "#panel-test-5" ),
- panel6 = $( "#panel-test-6" ),
- panel7 = $( "#panel-test-7" ),
- panel8 = $( "#panel-test-8" ),
- panel9 = $( "#panel-test-9" ),
- defaults = $.mobile.panel.prototype.options;
+ return $page.find( "." + defaults.classes.modal ).filter(function() {
+ return $( this ).data( "panelid" ) === $panel.attr( "id" );
+ });
+ }
+
+ function getWrapperFromPage( $page ) {
+ return $page.find( "." + defaults.classes.contentWrap );
+ }
+
+ $(document).on( "pageinit", function() {
@johnbender

johnbender Jan 3, 2013

Contributor

I would unwrap these tests and do something like:

// stop qunit from running the tests until everything is in the page
QUnit.config.autostart = false;

$( document ).on( "pageinit", function(){ 
  QUnit.start();
});
@zachleat

zachleat Jan 3, 2013

Contributor

Done in 5ab2ba6

Contributor

johnbender commented Jan 3, 2013

The only other comment I have is on the css and attribute testing. It's hard for me to tell but looks like we're testing all of the attributes in each test. We can probably just trim that down to the attributes that should be different between tests and a baseline that each attribute is set to the defaults in it's own test.

Great work @zachleat

Contributor

zachleat commented Jan 3, 2013

Hm, maybe I'm misunderstanding what you mean but the create and open and close methods are tested separately, but have some overlap in what classes are added and removed between them. There shouldn't be any testing duplication there.

zachleat added some commits Jan 3, 2013

@zachleat zachleat Moved the test code out of pageinit event handler and replaced with Q…
…unit.start()
5ab2ba6
@zachleat zachleat Remove the pageinit stuff, we're getting that for free from asyncLoad…
…. Saw some weird timing issues when reusing the same panel and asyncTests, so we're using different panels for each test (per @johnbender's recommendation).
6b9c017
Contributor

zachleat commented Jan 4, 2013

Okay, this should be ready to go now.

👍

@arschmitz arschmitz pushed a commit that referenced this pull request Jan 5, 2013

Alexander Schmitz Panel: updates from @zachleat PR #5412 help to resolve merge conflicts 5980205
Owner

arschmitz commented Jan 5, 2013

@zachleat @toddparker @scottjehl @jlembeck @ugomobi @johnbender @Wilto Panels are merged in master with these changes.

arschmitz closed this Jan 5, 2013

Contributor

toddparker commented on 676d192 Jan 7, 2013

@zachleat - we do need this CSS rule to make the swipe to close work. I wonder if we just need to do some sniffing get get transitionend to work in FF?
https://gist.github.com/4414792

Contributor

zachleat replied Jan 7, 2013

We can just delay adding the page-block class until after the transition finishes—fixes both issues.

#5422

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment