Skip to content
This repository has been archived by the owner on Jul 24, 2024. It is now read-only.

Adding support for passing a precision option to libsass #339

Merged
merged 2 commits into from
Jun 8, 2014

Conversation

johnobe
Copy link
Contributor

@johnobe johnobe commented Jun 7, 2014

This PR exposes the precision option and applies to issue #259.

@andrew
Copy link
Contributor

andrew commented Jun 7, 2014

This is looking good 👍, can you add a test for it as well and I'll merge it in.

Thanks!

@andrew andrew mentioned this pull request Jun 7, 2014
@johnobe
Copy link
Contributor Author

johnobe commented Jun 7, 2014

I've included one test as part of the PR (it's included with commit ee393). In the test below if you take out the precision option, or change it to a value less than 8 and you'll see a test failure.

The test doesn't cover the CLI though. If you want me to add a CLI test using a precision option, I can certainly do that. I wasn't sure how much testing was needed.

Thanks for taking a look at this so quickly!

describe('precision support', function() {
  it('should render when precision is specified', function(done) {
    sass.render({
      data: '.test { margin: 1.23456789 px; }',
      precision: 10,
      success: function(css) {
        done(assert.equal(css, '.test {\n  margin: 1.23456789 px; }\n')); 
      },
      error: function(error) {
        done(error);
      }
    });
  });
});

@andrew
Copy link
Contributor

andrew commented Jun 7, 2014

That's looking good, I'll get it merged in tomorrow hopefully. Thanks!

andrew pushed a commit that referenced this pull request Jun 8, 2014
Adding support for passing a precision option to libsass
@andrew andrew merged commit 86e5666 into sass:master Jun 8, 2014
@andrew
Copy link
Contributor

andrew commented Jun 8, 2014

Just tried this out, it's looking good 👍

@ckihneman
Copy link

@andrew I'm trying to get this running on my machine. I followed the steps from the readme for rebuilding binaries with no luck. Everything ran fine after I installed the sass2scss submodule in libsass, but the number precision is still defaulting to 5. Am I missing something?

And maybe the readme should have:

git clone https://github.com/sass/node-sass.git
cd node-sass
git submodule init
git submodule update
npm install
cd libsass
git submodule init
git submodule update
npm install -g node-gyp
cd ..
node-gyp rebuild

@andrew
Copy link
Contributor

andrew commented Jun 9, 2014

@ckihneman the build system/documentation is a bit of a mess right now, remove the binding.node from your platforms directory in bin/ and then run npm install again, running node-gyp on it's own doesn't copy the generated binary into the right place.

@ckihneman
Copy link

@andrew Heh, got it working. One major detail here ... the scripts/prepublish script was overriding my freshly created binaries. I simply commented out the last three lines of that file and it worked like a charm. It had me running loops for awhile.

Thanks very much for the help. Looking forward to this hitting npm.

For anyone on the outside looking in that wants to get this running now...

npm install -g node-gyp
cd node_modules
git clone https://github.com/sass/node-sass.git
cd node-sass
git submodule init
git submodule update
cd libsass
git submodule init
git submodule update

Comment out lines 31 - 33 of node_modules/node-sass/scripts/prepublish.

Then run npm install from node_modules/node-sass.

@andrew
Copy link
Contributor

andrew commented Jun 9, 2014

Thanks @ckihneman, I'd like to try and get #337 fixed as well before releasing a new version but debugging it is proving to be tricky!

@johnobe johnobe deleted the fix259 branch June 12, 2014 15:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants