Skip to content

Functionality for fallback values and Nested selectors - #24

Open
rtrufin wants to merge 17 commits into
ryanbahniuk:masterfrom
rtrufin:master
Open

Functionality for fallback values and Nested selectors#24
rtrufin wants to merge 17 commits into
ryanbahniuk:masterfrom
rtrufin:master

Conversation

@rtrufin

@rtrufin rtrufin commented Jun 1, 2016

Copy link
Copy Markdown

Test for Mixins that Nest Selectors #13.
Fixed issue with properties that have fallback.
E.g.:

padding: 16px;
padding: 1rem;

rtrufin added 3 commits June 1, 2016 11:41
Test for Mixins that Nest Selectors ryanbahniuk#13.
Fixed issue with properties that have fallback.
E.g.: padding: 16px;
padding: 1rem;
@ryanbahniuk

Copy link
Copy Markdown
Owner

Thanks for the PR, will check this out this weekend.

@ryanbahniuk

Copy link
Copy Markdown
Owner

Can you add integration and unit tests for the new features?

@rtrufin

rtrufin commented Jun 6, 2016

Copy link
Copy Markdown
Author

Sure, I'll start working on them this week, Also, I'll test the deepEqual assertion. Thanks for the reply

@rtrufin

rtrufin commented Jun 9, 2016

Copy link
Copy Markdown
Author

I have some issues while running the tests, on the mixinResult and FuncResult test files:

FuncResult
    wrapFunctionWithArgs
      1) "before each" hook for "wraps the function and args with the necessary SCSS to not fail the compiler"

MixinResult
    wrapMixinWithArgs
      2) "before each" hook for "wraps the standalone mixin to not fail the compiler"

these two errors block all the tests from those files from running

@ryanbahniuk

Copy link
Copy Markdown
Owner

I checked out your branch and got all the tests to pass on Node 0.12.0 and Node 5.11.0. However, it is possible that your problem is due to the Sass binaries failing to install during npm install. I would confirm you are on at least 0.12.0 and then delete your node modules folder, and retry installing the dependencies.

@pvdlg

pvdlg commented Jul 11, 2017

Copy link
Copy Markdown

Hello,
is there any plan to merge this PR? It would be really useful.
Thanks

@rtrufin

rtrufin commented Jul 11, 2017

Copy link
Copy Markdown
Author

@vanduynslagerp (totally forgot about this PR) I'm going to work on it later this week. I've had the issues with the unit tests and I'm trying to see what I can do about that

@rtrufin

rtrufin commented Oct 19, 2017

Copy link
Copy Markdown
Author

Fixed the tests. The issue was caused by the paths that were not concatenated properly. Added the normalize-paths module to handle that issue.

Comment thread src/sassaby.js Outdated
for (var variableName in varz) {
sassVariables = sassVariables + '$' + variableName + ':' + varz[variableName] + ';';
}
return sassVariables;

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Can you undo the indentation changes to this file so the diff reads cleaner?

Comment thread README.md Outdated
}
```
```js
sassaby.includedMixin('font-size').calledWithArgs('2').declaresProperties('max-height', ['32px', '2rem']);

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I think I'd rather have this functionality in the current API. I could see declares taking either a string or an array. If it is an array with one item it will echo current behavior and if its an array with multiple items it will make the fallback assertion defined here.

@rtrufin

rtrufin commented Oct 25, 2017

Copy link
Copy Markdown
Author

I've fixed all the issues, and it seems that I also had some issues with the coverage. It is now at 100%.
Also, I'll probably start working on another branch in order to make the module work on newer versions of node.

@ryanbahniuk

Copy link
Copy Markdown
Owner

Looks good. Did you see my previous comment (got collapsed by GitHub) that I think the new declaresProperties behavior should actually just live in the existing declares method. I can see that method taking 3 types of arguments:

string - current behavior which will stay around for backwards compatibility
array - if it has 1 item it will have very similar behavior to the string. if it has more than 1 item it will assert on a fallback like you have in your declaresProperties method.

What do you think about that?

@rtrufin

rtrufin commented Oct 27, 2017

Copy link
Copy Markdown
Author

@ryanbahniuk I've changed declares to work both with arrays and Strings and removed the declaresProperties method. See: mixinResult.js

@ryanbahniuk

Copy link
Copy Markdown
Owner

@rtrufin looks good. Can you make sure the docs match the new updates?

declaresProperties has been merged into the declares method
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants