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

Always define compact #86

Merged
merged 1 commit into from
May 14, 2016
Merged

Always define compact #86

merged 1 commit into from
May 14, 2016

Conversation

xzyfer
Copy link
Collaborator

@xzyfer xzyfer commented Apr 23, 2016

This implement of compact is functionally equivalent of the
unofficial native one that has been permanently removed from LibSass.
There is no risk in always using this version. The version of LibSass
that had a compact function is extremely difficult to obtain, and
is long unsupported.

More importantly defining function inside control structures is not
valid in Sass and produces errors for people using this library.

Closes #73
Fixes #84
Fixes #85

This was referenced Apr 23, 2016
@alebelcor
Copy link

I can't see why the build failed in Travis. Any clues?

robertknight added a commit to hypothesis/h that referenced this pull request Apr 28, 2016
Fix build by pinning node-sass dependency (used indirectly via
gulp-sass) to v3.4.x until
Igosuki/compass-mixins#86 is resolved.
@xzyfer
Copy link
Collaborator Author

xzyfer commented May 4, 2016

The transition mixin has always been broken in Sass. I've updated so it now works as expected on both Node Sass and Sass.

This implement of compact is functionally equivalent of the
unofficial native one that has been permanently removed from LibSass.
There is no risk in always using this version. The version of LibSass
that had a `compact` function is extremely difficult to obtain, and
is long unsupported.

More importantly defining function inside control structures is not
valid in Sass and produces errors for people using this library.

Fixes Igosuki#84
Fixes Igosuki#85
@tjenkinson
Copy link

Yes please can this be merged?

Refs: clappr/clappr#970

@tjenkinson
Copy link

tjenkinson commented May 7, 2016

FYI I've created a fork with this fix: https://github.com/tjenkinson/compass-mixins (because it looks like this might not be maintained anymore).

@lemoustachiste
Copy link

This is a blocker for me, I have fixed my initial issue by installing node-sass 3.7.0 but this remains. Is this going to get merged and released?

@phazei
Copy link

phazei commented May 11, 2016

@Igosuki please merge these, many projects depend on it, thank you

@shri3k
Copy link

shri3k commented May 12, 2016

Agreed with @phazei. I don't know why node-sass didn't mark this as breaking change. @tjenkinson, I'm guessing your fork hasn't been published to npm has it? (even though I can use github's link in my package.json I'm a little lazy. 😜 )

@xzyfer
Copy link
Collaborator Author

xzyfer commented May 12, 2016

As far as node-sass is concerned it wasn't a breaking change. You can read
more about why in
https://medium.com/@xzyfer/why-node-sass-broke-your-code-and-semver-1b3e409c57b9#.av46a780g
On 13 May 2016 8:18 AM, "Yojan Shrestha" notifications@github.com wrote:

Agreed with @phazei https://github.com/phazei. I don't know why
node-sass didn't mark this as breaking change. @tjenkinson
https://github.com/tjenkinson, I'm guessing your fork hasn't been
published to npm has it? (even though I can use github's link in my
package.json I'm a little lazy. 😜 )


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#86 (comment)

@tjenkinson
Copy link

tjenkinson commented May 12, 2016

@shri3k yes I just did

npm install tjenkinson-compass-mixins

https://www.npmjs.com/package/tjenkinson-compass-mixins

Hopefully this will start being maintained again though.

@shri3k
Copy link

shri3k commented May 12, 2016

@xzyfer Ah, that sort of clarifies things.
@tjenkinson Nice. Thank you and agreed I'd rather like this to be maintained instead.

@xzyfer xzyfer merged commit e3d7d67 into Igosuki:master May 14, 2016
@xzyfer xzyfer deleted the patch-1 branch May 14, 2016 03:53
@shri3k
Copy link

shri3k commented May 16, 2016

@xzyfer Thank you for merging. I'm confirming that this works. 🎉

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.

6 participants