HTML Reporter: Multi-select module dropdown replacing module selector #973
Conversation
|
Excited for this! I'll try to take a look sometime this weekend and mess around with it locally. |
|
Nice, I'll try to test it locally and provide feedback :) |
| if ( modulesList[ i ].checked ) { | ||
| selectedModules.push( modulesList[ i ].id ); | ||
| } | ||
| } |
There was a problem hiding this comment.
These changes should wait for gh-969 to land, and then rebase on top of it.
|
One general comment, take a look at the jQuery CSS Style Guide, you have some minor issues in the CSS. Such as units for 0 values and non-hex color values. |
| return false; | ||
| } | ||
|
|
||
| moduleFilter.setAttribute( "id", "qunit-modulefilter-container" ); | ||
| moduleFilter.innerHTML = moduleFilterHtml; | ||
| moduleSearch.placeholder = "Select modules"; |
There was a problem hiding this comment.
placeholder is not supported in our current browser policy. Another notch for waiting for 2.0.
There was a problem hiding this comment.
this should just progressively fall back to not doing anything though right?
There was a problem hiding this comment.
Next release will be 2.0, so that won't be an issue
|
Hi @mlato-yahoo
|
This is a fair behavior. The dropdown selector could replace the |
Yeah, I forgot that any test passing all filters ( I was thinking that, it should run one module if But the current behavior is OK. and seems legit. |
|
screenshot:
|
mlato-yahoo
force-pushed
the
multi-select-module-filter
branch
from
b073b15
to
850958d
Compare
|
|
||
| // Remove moduleId and testId filters | ||
| moduleId: undefined, | ||
| // Remove testId filter |
There was a problem hiding this comment.
The old code set module and filter, and cleared moduleId and testId. If we're now setting moduleId instead of module, we should clear module.
mlato-yahoo
force-pushed
the
multi-select-module-filter
branch
5 times, most recently
from
1afed93
to
a2379b1
Compare
mlato-yahoo
changed the title
mlato-yahoo
force-pushed
the
multi-select-module-filter
branch
from
a2379b1
to
f2d2614
Compare
|
I've fixed the major CSS issues and changed most units to use |
|
The sizing should also be fixed - menus align properly on all browsers. |
|
This looks good to go, but it doesn't need to come along with 2.0.0 or this means it doesn't bring any breaking change, am I right? |
I guess that would depend on if we consider the DOM public API. If anyone has extended QUnit in a way that makes assumptions about the module filter's DOM, then they would break. I wouldn't mind seeing this implemented in 2.0. |
|
2.0 changes beyond deprecated methods should be minimal to avoid complex migrations. But that doesn't seem to go with 2.0 and cause problems. |
|
We've made plenty of changes to the HTML reporter over the various 1.x releases, seems fine to land this in any release. |
Well that answers my question then. I'm good with this landing whenever it is ready then; let's not block 2.0 on it. |
| } | ||
|
|
||
| fn.call( elem, event ); | ||
| } ); |
|
merged at 0780127, thanks! |
Closes qunitjsgh-973 Replaces the old module selector.
A multi-select module dropdown, in response to #953