Package: Include structure CSS in themes zip#300
Package: Include structure CSS in themes zip#300scottgonzalez wants to merge 2 commits intojquery:masterfrom
Conversation
Changes the 1.12 themes packager to be built on top of the 1.12 packager. Fixes jquerygh-298
lib/package-1-12-themes.js
Outdated
| } | ||
|
|
||
| extend( Package.prototype, Package1_12.prototype ); | ||
| // Copy some properties from the Package1_12's prototype |
There was a problem hiding this comment.
Maybe add a comment why "inheritance" doesn't work?
|
👍 |
lib/package-1-12-themes.js
Outdated
| } ) | ||
| ).forEach( function( prop ) { | ||
| Package.prototype[ prop ] = Package1_12.prototype[ prop ]; | ||
| }); |
There was a problem hiding this comment.
How big would it be to explicitly list all the properties? Explicit code is easier to understand and maintain... Something like:
extend( Package.prototype, {
"AUTHORS.txt": Package1_12[ "AUTHORS.txt" ],
...
"themes": function( callback ) {There was a problem hiding this comment.
It wouldn't be hard, but it would be more error prone. This says to copy all CSS files, regardless of which files may exist.
There was a problem hiding this comment.
There's something in the current code that I believe could be simpler to look and understand at a glance. Perhaps extending the Package in one step for the css ones, and then:
extend( Package.prototype, {
"AUTHORS.txt": Package1_12[ "AUTHORS.txt" ],
"LICENSE.txt": Package1_12[ "LICENSE.txt" ],
"images": Package1_12[ "images" ],
"themes": function( callback ) {
...Feel free to completely ignore this whole comment though. :)
It's up to you.
|
Updated based on @rxaviers comment. |
|
👍 |
|
Works for me |
Changes the 1.12 themes packager to be built on top of the 1.12 packager.
Fixes gh-298