jackw / eyeglass-image-dimensions

An eyeglass module giving node-sass the same image dimension functionality found in ruby compass.
Apache License 2.0
4 stars 0 forks source link

Joanna's feedback #1

Closed jackw closed 8 years ago

jackw commented 9 years ago

In eyeglass-exports.js line 47, there's no need to loop through each module's assets to get the filepath. Since asset is a map, a map lookup using asset.coerce.get(file) should get the necessary information.

eg-image-width and eg-image-height should each return a Sass number rather than a string. This can be done using sassUtils.castToSass.

eg-image-dimensions should return a space-delimited Sass list. This can be done by calling sassUtils.castToSass on a 2-element array of SassDimensions with the separator set to false.

The convention we've been using is to import eyeglass modules without the eyeglass- prefix. This would just require adding an eyeglass field to package.json and setting the name to image-dimensions.

If the user passes in a real file path rather than an asset path, we should check that it's an absolute rather than a relative file path.

Update the readme to include image-dimensions.

Let's add some tests for the module.

jackw commented 9 years ago

Hey @joannajw Based on your feedback I made quite a few changes to my eyeglass image dimensions plugin and wondered if you'd have the time to look over it again? I added logic to check for an absolute vs relative file path but I'm a little hazy on how the asset paths are working and how to fully test them. Tests are (embarrassingly) something I'm not really experienced with, none the less I've given them a go!