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.

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

First Take w/ Github Image #7

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

First Take w/ Github Image #7

wants to merge 17 commits into from

Conversation

brh55
Copy link
Member

@brh55 brh55 commented Jan 13, 2018

Decided to take a first stab of enhancing pre-existing work in the PR #6. This PR mainly is using this repository as a way to provide image to directly referenced (against ToS? not sure 👯‍♂️ ), as well as additional minor improvements. Feel free to revise / update anywhere accordingly

Changes Include:

  • Optimizes build to skip pre-existing images
  • Adds a compressed 120 x 120 Icon version of the full image
  • References raw GitHub image and icon locations within JSON (This can be kept / remove up to you)
  • Restructures the cryptocurrencies.json file to accommodate for changes (open to changing property names)
  • Removes the list of currencies within the readme (was getting a bit too long when landing on the repository), but now moves this over to list.md, which now contains the icon images

Let me know what you think @radiovisual

@brh55 brh55 changed the title First Take In Github Image First Take w/ Github Image Jan 13, 2018
Copy link
Member

@radiovisual radiovisual left a comment

Choose a reason for hiding this comment

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

Looks good to me. The only thought I have is that maybe we offer two JSON files, one with the image data, and one without? This repo is about getting a list of cryptocurrencies, the images are just a bonus, but I think some use cases won't require the extra bloat from the image urls. What do you think

build.js Outdated
const ImageFile = `${Name}.${extension}`;
const ImagePath = path.join('images', ImageFile);

if (fs.existsSync(ImagePath)) {
Copy link
Member

@radiovisual radiovisual Jan 13, 2018

Choose a reason for hiding this comment

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

maybe we need to add a --force flag to the build then, because this checks for an existing image, but sometimes the icons change, and we will want a way to grab the updated images. We could get fancy with checksums, but it might be overkill.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes that's what I was thinking, a force flag should be beneficial.

@brh55
Copy link
Member Author

brh55 commented Jan 14, 2018

Looks good to me. The only thought I have is that maybe we offer two JSON files, one with the image data, and one without? This repo is about getting a list of cryptocurrencies, the images are just a bonus, but I think some use cases won't require the extra bloat from the image urls. What do you think

Agreed, let's go ahead and do that. How about the API implementation (.BTC vs .BTC.name)?

@radiovisual
Copy link
Member

How about the API implementation (.BTC vs .BTC.name)?

.BTC 👍

@brh55
Copy link
Member Author

brh55 commented Jan 15, 2018

Suggestions for the name of 2nd json file with images?

@radiovisual
Copy link
Member

Suggestions for the name of 2nd json file with images?

Ideas:

  • currencies-logos.json
  • currencies-meta.json (future-proofing, but semantic)
  • currencies-plus.json (future-proofing, but semantic)

@brh55
Copy link
Member Author

brh55 commented Jan 28, 2018

@radiovisual Updated per PR comments.

  1. Force flag (--force, force, -f, -force) will skip optimization and download all files
    • npm run build will be entire build process
    • npm run fast-build will be an optimized build process
  2. cryptocurrencies.json and API will remain intact
  3. cryptocurrencies-meta.json will be available for download

@radiovisual
Copy link
Member

radiovisual commented Jan 28, 2018

Looks good to me. My main issue is with the GitHub hosting. If anyone is going to use this in a real application they would want the images delivered by a cdn, and to be hosted in a way that is scalable and reliable. So maybe we add a note (disclaimer) that the GitHub hosting is for reference only, but give them a build command to specify where the images should be saved, and generate their own custom JSON file with the updated paths, then we can recommend that they maintain their own build process in their own apps This would free us from having to maintain a database of these images (that would break someone's app every time we updated the list of images 😆 ), but give users the flexibility to do what they want with the images without the right to yell at us. 🤣

@brh55
Copy link
Member Author

brh55 commented Jan 28, 2018

@radiovisual That's an excellent point. How about we include a build to generate a meta file for the images stored locally, and the current meta (will need to rename) file used only for the purpose of this repository's list.md for visual reference purposes?

@radiovisual
Copy link
Member

radiovisual commented Jan 28, 2018

How about we include a build to generate a meta file for the images stored locally, and the current meta (will need to rename) file used only for the purpose of this repository's list.md for visual reference purposes?

I like the idea of letting them generate their own metafile with references to the images, which forces them to maintain the images and the list on their own, and in this case, we don't even need to generate the meta JSON file at all, because if all we are doing is hosting the images to show in the list.md file, then we only need a file path for that in the .md file, which means we don't need to create a JSON file. So I vote for:

  1. Remove the META JSON file completely
  2. Use the build process to gather the images to the images dir
  3. Automatically add the image path to the list.md file
  4. Add a disclaimer to the list.md file that the images should be used for reference only, and can't be trusted as reliable links to images for their own apps as the icons can and will change without notice, and....
  5. Instruct users how to use the custom build process that that will save the images locally and generate a custom META JSON file for them to use in their own apps (in the README.md and LIST.md files)
  6. Expose the configuration flags to the build cli letting users generate the icons and custom meta JSON.

This way, they only thing we officially support is the somewhat up-to-date list of cryptocurrencies, with the build tool that lets users gain easy access to the image icons to use however they see fit...and we are pretty clear every step of the way that we aren't trying to maintain a versioned icon reference for anyone...since that would quickly become a nightmare to maintain.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants