Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Added hex colors to manifest.json #294

Merged
merged 8 commits into from
Apr 10, 2019
Merged

Added hex colors to manifest.json #294

merged 8 commits into from
Apr 10, 2019

Conversation

smokes
Copy link
Contributor

@smokes smokes commented Apr 9, 2019

Thought it would be useful to have the hex colors in manifest.json, also included a script to grab the colors from the svgs

@lukechilds
Copy link
Collaborator

This looks like a great contribution, thanks!

Does your script pass the linting rules?

You can test with npm test.

@lukechilds
Copy link
Collaborator

Also, I think the colour picking should just be added to scripts/manifest.js rather than having two separate scripts.

@smokes
Copy link
Contributor Author

smokes commented Apr 10, 2019

I'm going to add the color script to manifest.json and fix the linting errors

@smokes
Copy link
Contributor Author

smokes commented Apr 10, 2019

When trying to commit, the svgo script throws out this error

C:\Users\Smokie\Desktop\Code\crypto-icons\cryptocurrency-icons>node  "C:\Users\Smokie\Desktop\Code\crypto-icons\cryptocurrency-icons\node_modules\.bin\\..\svgo\bin\svgo" --multipass --disable=removeViewBox $(globby {svg,originals}/**/*.svg)
Error: Error: no such file or directory 'C:\Users\Smokie\Desktop\Code\crypto-icons\cryptocurrency-icons\$(globby'.

C:\Users\Smokie\Desktop\Code\crypto-icons\cryptocurrency-icons>npm ERR! code ELIFECYCLE
npm ERR! errno 255
npm ERR! cryptocurrency-icons@0.11.0 clean: `svgo --multipass --disable=removeViewBox $(globby {svg,originals}/**/*.svg)`

@lukechilds
Copy link
Collaborator

Ahh, that's just the shell command substitution failing because you're running it on Windows.

You should be able to remove the husky property from package.json to allow you to commit. (Don't actually commit removing the property, git add your files, then remove it).

@smokes
Copy link
Contributor Author

smokes commented Apr 10, 2019

Should i just delete the repository and recommit everything in a single commit to avoid the commit spam ?

lukechilds
lukechilds requested changes Apr 10, 2019
View reviewed changes
Copy link
Collaborator

@lukechilds lukechilds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good stuff, some small tweaks.

scripts/manifest.js Outdated Show resolved Hide resolved
scripts/manifest.js Outdated Show resolved Hide resolved
scripts/manifest.js Outdated Show resolved Hide resolved
scripts/manifest.js Outdated Show resolved Hide resolved
@lukechilds
Copy link
Collaborator

Should i just delete the repository and recommit everything in a single commit to avoid the commit spam ?

No, don't worry about that, I'll squash these into a single commit when it gets merged.

@smokes
Copy link
Contributor Author

smokes commented Apr 10, 2019

All done.

lukechilds
lukechilds requested changes Apr 10, 2019
View reviewed changes
scripts/manifest.js Outdated Show resolved Hide resolved
@smokes
Copy link
Contributor Author

smokes commented Apr 10, 2019

Fixed..

@lukechilds lukechilds merged commit 7be9023 into spothq:master Apr 10, 2019
@lukechilds
Copy link
Collaborator

lukechilds commented Apr 10, 2019 •
edited
Loading

@smokes Thanks, great contribution!

Published as cryptocurrency-icons@0.12.0.

sindresorhus
sindresorhus reviewed Apr 17, 2019
View reviewed changes
scripts/manifest.js
const fileName = `${id.toLowerCase()}.svg`;
const svgPath = path.resolve(__dirname, '../svg/color/', fileName);
let color;
if (fs.existsSync(svgPath)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the point of this check. If an icon is in the manifest, it should be guaranteed to exist. If it does not, it should fail loudly, not silently.

sindresorhus
sindresorhus reviewed Apr 17, 2019
View reviewed changes
scripts/manifest.js
if (fs.existsSync(svgPath)) {
const svg = fs.readFileSync(svgPath, 'utf8');
const fillColor = getColors(svg).fills[0];
color = fillColor ? fillColor.hex().toUpperCase() : undefined;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, we should fail loudly (meaning throw an error) if the color cannot be found, as that's definitely something we'll want to look into.

sindresorhus
sindresorhus reviewed Apr 17, 2019
View reviewed changes
scripts/manifest.js
@@ -44,10 +45,19 @@ const overrides = new Map([

const icons = manifest.map(icon => {
const id = typeof icon === 'string' ? icon : icon.symbol;
const fileName = `${id.toLowerCase()}.svg`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filename is a word, so should have been filename, not fileName.

@sindresorhus
Copy link
Contributor

TGCH does not have a color when running npm run manifest to generate the manifest. Did you add it manually?

sindresorhus added a commit that referenced this pull request Apr 17, 2019
@sindresorhus
Fix #294
5a160a8
@sindresorhus
Copy link
Contributor

Everything fixed in 5a160a8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sindresorhus sindresorhus sindresorhus left review comments

@lukechilds lukechilds lukechilds requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@smokes @lukechilds @sindresorhus