-
Notifications
You must be signed in to change notification settings - Fork 13
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
Scope option #16
Scope option #16
Conversation
README.md
Outdated
@@ -65,7 +65,7 @@ builder.build({ | |||
|
|||
Creates a new `Builder` instance. | |||
|
|||
It caches build results to only rebuild a theme when related files have been changed. |
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.
Did you remove those by intension? Actually I've put the two spaces to have a line-break.
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.
Oh sorry, this probably was changed on save, I didn't notice as I only checked the source files when creating the PR.
lib/index.js
Outdated
@@ -442,6 +454,10 @@ Builder.prototype.build = function(options) { | |||
|
|||
// Compile theme if not cached or RTL is requested and missing in cache | |||
if (!themeCache || (options.rtl && !themeCache.result.cssRtl)) { | |||
var scopeOptions = options.scope; | |||
if (scopeOptions.selector && scopeOptions.embeddedFilePath && scopeOptions.embeddedCompareFilePath && scopeOptions.baseFile) { |
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.
This check also needs to be done in the else
clause below, where currently only readDotTheming(dotThemingInputPath).then(addInlineParameters).then(that.cacheTheme.bind(that));
is called (line 467)
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.
Done!
lib/index.js
Outdated
@@ -442,6 +454,10 @@ Builder.prototype.build = function(options) { | |||
|
|||
// Compile theme if not cached or RTL is requested and missing in cache | |||
if (!themeCache || (options.rtl && !themeCache.result.cssRtl)) { | |||
var scopeOptions = options.scope; | |||
if (scopeOptions.selector && scopeOptions.embeddedFilePath && scopeOptions.embeddedCompareFilePath && scopeOptions.baseFile) { | |||
return compileWithScope(scopeOptions); |
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.
What about addInlineParameters
and cacheTheme
? Those won't be called in this case, right?
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.
They should probably be added too, you're right.
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.
Thank you for your review, @matz3. I added the requested changes!
README.md
Outdated
@@ -65,7 +65,7 @@ builder.build({ | |||
|
|||
Creates a new `Builder` instance. | |||
|
|||
It caches build results to only rebuild a theme when related files have been changed. |
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.
Oh sorry, this probably was changed on save, I didn't notice as I only checked the source files when creating the PR.
lib/index.js
Outdated
@@ -442,6 +454,10 @@ Builder.prototype.build = function(options) { | |||
|
|||
// Compile theme if not cached or RTL is requested and missing in cache | |||
if (!themeCache || (options.rtl && !themeCache.result.cssRtl)) { | |||
var scopeOptions = options.scope; | |||
if (scopeOptions.selector && scopeOptions.embeddedFilePath && scopeOptions.embeddedCompareFilePath && scopeOptions.baseFile) { | |||
return compileWithScope(scopeOptions); |
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.
They should probably be added too, you're right.
lib/index.js
Outdated
@@ -442,6 +454,10 @@ Builder.prototype.build = function(options) { | |||
|
|||
// Compile theme if not cached or RTL is requested and missing in cache | |||
if (!themeCache || (options.rtl && !themeCache.result.cssRtl)) { | |||
var scopeOptions = options.scope; | |||
if (scopeOptions.selector && scopeOptions.embeddedFilePath && scopeOptions.embeddedCompareFilePath && scopeOptions.baseFile) { |
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.
Done!
@marikaner thanks! |
I did some final changes which required me to rename I'm planning to release v0.4.0 later today. |
This option allows to use the builder while explicitly defining how to scope the ruleset without .theming file.