Gravatar refactor #45
Conversation
- Added eslint and editorconfig files ( you can change it as you prefer) - Changed 'var' into 'const' or 'let' - Decoupled printAvatar into 2 functions for url and for profile - Changed if/else into if/ return pattern in most cases - String concatination replaced with interpolation in most cases
- remove `package-lock.json`, use yarn
- bundling with rollup - updating `package.json` - updating tests - setup linter - updating ci config
vladimyr
force-pushed
the
gravatar-refactor
branch
from
9dfeaab
to
44d558d
Compare
vladimyr
force-pushed
the
gravatar-refactor
branch
from
5d0770d
to
fbf0a92
Compare
vladimyr
force-pushed
the
gravatar-refactor
branch
from
ca44924
to
b4926e2
Compare
vladimyr
force-pushed
the
gravatar-refactor
branch
from
b4926e2
to
1892911
Compare
|
👌 Very nice! |
- listing jsonp `callback` param for profile url
- updating readme
| if (isBoolean(protocol)) { | ||
| url = normalizeUrl(url, { forceHttps: protocol }); | ||
| } | ||
| } |
There was a problem hiding this comment.
How about this:
let url = cdn ? urlJoin(cdn, '/avatar') : urlJoin(baseUrl, '/avatar');
// could `let url = urlJoin(cdn || baserUrl, '/avatar');` work?
if (!cdn && isBoolean(protocol)) {
url = normalizeUrl(url, { forceHttps: protocol });
}
packages/gravatar/src/gravatar.ts
Outdated
| url = `${cdn}`; | ||
| } else { | ||
| url = normalizeUrl(baseUrl, { forceHttps: Boolean(protocol) }); | ||
| } |
There was a problem hiding this comment.
Similiar as above:
const url = cdn ? cdn : normalizeUrl(baseUrl, { forceHttps: Boolean(protocol) }); maybe
const url = cdn || normalizeUrl(baseUrl, { forceHttps: Boolean(protocol) });would work also?
cdn is string so template literals are obselete.
|
@emerleite ping :) |
|
Hey guys, sorry for the delay. Can you wait til the weekend to the review? I'll generate a new version using your contribution. Thanks :) |
- upgrading dependencies
- updating tests
|
This PR will also fix vulnerability https://bugzilla.redhat.com/show_bug.cgi?id=1623744 generated by yargs > os-locale > mem and fixed with yargs > 13.0.0. |
|
Perfect @PorresM. Sorry for the delay. I'll try to check this weekend. |
|
any update? would be nice to support this as rollup -c doesnt support current version when using es6 |
|
Ping. |
- addressing lint issues
|
Hey guys, do you know if this PR fixes this vulnerability: https://npmjs.com/advisories/1500 Also @emerleite would it be possible to get this PR reviewed? I personally would love to see this PR merged. |
This PR refactors gravatar packet into ES6 and also polishes code.