HatScripts / circle-flags

A collection of 400+ minimal circular SVG country, state and language flags
https://hatscripts.github.io/circle-flags
MIT License
986 stars 256 forks source link

Add package.json & instructions how to install with NPM #15

Closed jussikinnula closed 3 years ago

jussikinnula commented 3 years ago

Rationale behind this pull request is that we have React Native project, and we want to ship the circle flags that are used in the application itself (e.g. we don't want to make request to Internet for the flags for sure). By creating package.json on this project allows adding it as a npm dependency, and using the circle flags from filesystem. We of course could have used this project as a GIT submodule, but handling dependencies via npm is somewhat better.

I could contribute to the community by creating a React Native package as well if we can get the dependency thing merged in! :-)

waldyrious commented 3 years ago

@jussikinnula please correct me if I'm wrong, but wouldn't the react-circle-flags package (linked from the README) serve the purposes you outline here? Or wouldn't it otherwise overlap with this project if this PR is merged?

jussikinnula commented 3 years ago

@jussikinnula please correct me if I'm wrong, but wouldn't the react-circle-flags package (linked from the README) serve the purposes you outline here? Or wouldn't it otherwise overlap with this project if this PR is merged?

I wanted to specifically explain myself in the description, why this change is needed. But anyway I'm happy to clarify more!

That react-circle-flags project works only for websites which can use external CDN to deliver the flag image files, since it uses https://hatscripts.github.io/circle-flags/flags/ as CDN. You can alternatively manually put the same files to your website and device CDN_URL for that. And more specifically, react-circle-flags uses <img src=... /> for the images, which works on some web development cases, but not with React Native.

With React Native one can display Svg's with Image or react-native-svg. For example we use import FlagDeIcon from 'circle-flags/flags/de.svg'; and then just <FlagDeIcon /> in JSX code. Alternatively we could use import { Image } from 'react-native'; and put the imported *.svg into it, but since we use react-native-svg it automatically transforms imported SVG files into React components, so it wouldn't be practical on our case.

Anyway, with this change it would be also possible to enhance react-circle-flags to use circle-flags as dependency, and import the SVG files instead of relying manually to copy them or use CDN.

waldyrious commented 3 years ago

Great, thanks so much for the detailed explanation! I would suggest, then, to expand the "React" section in the README in this PR (or keep the new content in a separate NPM section if you think that's best), to explain precisely what you say above, so people will know when and how to use each approach.

tnovau commented 3 years ago

Hi @jussikinnula I am the owner and maintainer of react-circle-flags. Would you like to put that code into the package?

It will be very helpful, and I will be happy to receive your PR

jussikinnula commented 3 years ago

@tnovau & @waldyrious, I'll make a PR for react-circle-flags. I could make that PR non-breaking change as a new property, for example "useLocalImport" (or if you can come up with a better name).

In technical terms I have in current project prebuild-hook which generates list of valid imports:

import path from 'path';
import fs from 'fs';
import pascalcase from 'pascalcase';

const flagsPath = path.resolve(__dirname, '../node_modules/circle-flags/flags');

if (!fs.existsSync(flagsPath)) {
  console.error('Flags path does not exist!')
  process.exit();
}

const flagFiles = fs.readdirSync(flagsPath).filter((flag: string) => flag.endsWith('.svg'));

const flags = flagFiles.map(flagFile => {
  const name = flagFile.split('.svg')[0];
  const importName = pascalcase(`flag-${name}`);
  const importLine = `import ${importName} from 'circle-flags/flags/${flagFile}';`
  return { importName, importLine, name };
});

// ...rest of the generation script

This ensures that Webpack/Parcel/other build tools can safely import necessary file. It's also possible that I could put this script in circle-flags side (even on this PR), so it would generate index.js can importing from circle-flags would become then easier. I can also make it as a separate PR.

And maybe this is also perfect place to introduce also react-native-circle-flags package, so that we cover all of the use-cases.

HatScripts commented 3 years ago

Hi @jussikinnula, thanks for your interest in the project. Just to clarify, as I'm not all that familiar with React (and by extension React Native), is this PR still relevant or useful to this repo given that you said you'd make a PR to @tnovau's react-circle-flags wrapper?

jussikinnula commented 3 years ago

Hi @jussikinnula, thanks for your interest in the project. Just to clarify, as I'm not all that familiar with React (and by extension React Native), is this PR still relevant or useful to this repo given that you said you'd make a PR to @tnovau's react-circle-flags wrapper?

@HatScripts, yes. Adding package.json to this repository would enable to use it as dependency in other projects. Preferably this should be published also in npmjs.com.

Without packaging one could use this as a GIT submodule or like react-circle-flags project does - it uses by default images served in GitHub CDN. Optimally with this project using package.json I could (and would) make a PR to support bundling circle-flags automatically via JavaScript imports, which the preferred approach nowadays.

HatScripts commented 3 years ago

@jussikinnula Okay, that makes sense. Thanks very much. I will merge this PR and look forward to seeing your future contributions.

I would suggest, then, to expand the "React" section in the README in this PR (or keep the new content in a separate NPM section if you think that's best), to explain precisely what you say above, so people will know when and how to use each approach.

I also think what @waldyrious suggested above is a good idea.

waldyrious commented 3 years ago

I also think what @waldyrious suggested above is a good idea.

I was hoping that would be done in this PR, actually :) but a new one is fine. @jussikinnula are you up for it? I would do it myself but I lack the background to make sure I don't add or omit anything that would be incorrect or misleading.