Skip to content
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

Merged
merged 6 commits into from
Dec 13, 2017
Merged

Scope option #16

merged 6 commits into from
Dec 13, 2017

Conversation

marikaner
Copy link
Contributor

This option allows to use the builder while explicitly defining how to scope the ruleset without .theming file.

@matz3 matz3 self-assigned this Nov 2, 2017
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.
Copy link
Member

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.

Copy link
Contributor Author

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) {
Copy link
Member

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)

Copy link
Contributor Author

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);
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

@marikaner marikaner left a 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.
Copy link
Contributor Author

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);
Copy link
Contributor Author

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) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@matz3
Copy link
Member

matz3 commented Nov 28, 2017

@marikaner thanks!
I've added tests that use this new option and found an issue (tests are failing). I will have a look into it and get in contact with you as it's related to some special handling I guess you weren't aware of.

@matz3
Copy link
Member

matz3 commented Dec 13, 2017

I did some final changes which required me to rename scope.baseFile to scope.baseFilePath.

I'm planning to release v0.4.0 later today.
Thanks again for your contribution 👍

@matz3 matz3 merged commit af674a8 into SAP:master Dec 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants