Bhavya, Kyler, Shawn bitmap transform!#27
Bhavya, Kyler, Shawn bitmap transform!#27bazacko wants to merge 40 commits intocodefellows-seattle-javascript-401d12:masterfrom
Conversation
rearranged into team folder, added placeholder gulpfile
documentation started
basic framework of bitmap transformer library, gulpfile, package.json
bnates
left a comment
bnates
left a comment
There was a problem hiding this comment.
Nice job on this project - there are a few nitpicks on the PR, feel free to review and let me know if you have any questions.
README.md
Outdated
| - chai | ||
| - gulp | ||
| - gulp-eslint | ||
| - gulp-mocha |
There was a problem hiding this comment.
@bazacko @bhavyaab @ShaAllFar nice job on the README.md!
| .pipe(eslint.failAfterError()); | ||
| }); | ||
|
|
||
| gulp.task('default', ['lint', 'test']); |
There was a problem hiding this comment.
@bazacko @bhavyaab @ShaAllFar gulpfile.js looks good!
| ourTB.invert().grayscale().colorScale(1, 1.3, 0.8).write('out.bmp'); | ||
| console.log('chained transform written to out.bmp'); | ||
|
|
||
| const chooseTransform = require('./lib/cli.js'); |
There was a problem hiding this comment.
@bazacko @bhavyaab @ShaAllFar good inclusion of the chooseTransform module in your index file - that said, not sure if you should include the ourTB module in this file, with the associated transforms - this should be abstracted out and the user should be able to perform transforms through the CLI or by simply running the app and the appropriate methods being called (just not in the index file)
| this.bitmap = fs.readFileSync(path); | ||
| this.colorTableOffset = 14 + this.bitmap.readUInt32LE(14); | ||
| //this.width = this.bitmap.readUInt32LE(18); | ||
| //this.height = this.bitmap.readUInt32LE(22); |
There was a problem hiding this comment.
@bazacko @bhavyaab @ShaAllFar best practice is to remove these comment blocks from the master branch
| }; | ||
|
|
||
|
|
||
| TransformableBitmap.prototype.write = function(path) { |
There was a problem hiding this comment.
@bazacko @bhavyaab @ShaAllFar transform methods look good!
| } | ||
| }); | ||
| } | ||
| }; |
There was a problem hiding this comment.
@bazacko @bhavyaab @ShaAllFar good job including the cli module yet this file is very procedurally written and could be condensed into a much smaller amount of code - too many if statements and hardcoded options
| bmp.horizontal_resolution = bitmap.readUInt32LE(38);//2834 | ||
| bmp.vertical_resolution = bitmap.readUInt32LE(42);//2834 | ||
| bmp.number_of_colors = bitmap.readUInt32LE(46);//256 | ||
| bmp.important_colors = bitmap.readUInt32LE(50);//256 |
There was a problem hiding this comment.
@bazacko @bhavyaab @ShaAllFar these should be created as a constructor instance and not directly on an object literal like the one defined above - that will allow for more modularity and reusability
| expect(property.colorTableOffset).to.equal(54); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
@bazacko @bhavyaab @ShaAllFar bitmap tests look good - that said, there could definitely be a few more that checks the output of a bitmap and the associated properties of a newly created bitmap
4 participants
No description provided.