update code to es6 #1
Conversation
|
@bharathvaj1995 - Appreciate you modernizing this library a bit. I've made note of a couple changes I'd like to see prior to merging. Style-wise:
|
| } | ||
| } | ||
|
|
||
| module.exports = (new RandomCat()); No newline at end of file |
There was a problem hiding this comment.
I'd leave the (new RandomCat()). By removing the new RandomCat() it changes the interface. This breaks compatibility and would require a major and/or minor version update.
There was a problem hiding this comment.
will change and push
| // Possible values: 50, 100, 150, 200, 250, 300, 350, 400, 450, 500, 550, 600 | ||
| // http://lorempixel.com/100/600/cats | ||
| var url = randomCat.get(); | ||
| var url = new randomCat.get(); |
There was a problem hiding this comment.
This is why I want to leave the new RandomCat(), to avoid all these new objects. If we were to keep it without the new RandomCat(), rather than what you've done here, it'd be more appropriate to do this:
const randomCat = new RandomCat();
const url = randomCat.get();
| options = options || {}; | ||
|
|
||
| if (options.dummyText) { | ||
| options.dummyText = escape(options.dummyText); |
| options.gray ? 'g' : false | ||
| , options.width || randomSizes[Math.ceil(Math.random() * 12)] | ||
| , options.height || randomSizes[Math.ceil(Math.random() * 12)] | ||
| , options.category || 'cats' |
There was a problem hiding this comment.
I believe you've lost the category and default here...'cats' being the default is pretty important
There was a problem hiding this comment.
since this is an exclusive random cat category ... why do we need to give category option?
There was a problem hiding this comment.
It's not a good thing to change what options you can pass. If someone had been using "Dogs" or anything else, it would break that.
You are right though, this is a random cat lib, so why give another option? I think in this case it is the principal of backward compatibility.
|
Just curious...how did you find this lib and what prompted you to rewrite it w/es6? Are you using this lib? I really appreciate you taking the time to modernize this, so thank you! |
2 participants
Bump the major version