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

Add option to prefix id attribute of symbol element #2

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

elidupuis
Copy link

Pretty self-explanatory... Lemme know what you think.

@tomwayson
Copy link

any reason not to merge this? I really need this. instead, I've had to add something like the following in order to add a prefix to all files viafileSettings:

  _createSvgStoreTree (iconFolder) {
    // prefix the file names with addon name and folder
    const srcDir = path.join(__dirname, `addon/assets/${iconFolder}`);
    const inputFiles = fs.readdirSync(srcDir);
    const fileSettings = inputFiles.reduce((acc, fileName) => {
      const key = fileName.replace(/\.svg$/, '');
      acc[key] = {
        id: `${this.name}-${iconFolder}-${key}`
      };
      return acc;
    }, {});
    // build the svgstore sprite
    return svgstore(new Funnel(srcDir), {
      outputFile: `${this.name}/assets/images/${iconFolder}.svg`,
      fileSettings,
      svgstoreOpts: {
        cleanSymbols: true
      }
    });
  },

I've had to add that to a couple addons now, and would prefer to have this just support a prefix option.

@tomwayson
Copy link

tomwayson commented Jan 29, 2018

OK, I now see the issue, this PR is quite old and seems to predate the new architecture. It would now be a matter of adding the prefix here:

var svgId = inputFileSettings.id || fileNameWithoutExtension;

@BrianSipple would you accept a new PR that adds a prefix option that (if passed in) will be prepended to the id at the above line?

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.

2 participants