laleksiunas / inline-svg-unique-id

Efficient ID generator at the runtime for inline SVG components
MIT License
18 stars 4 forks source link

Feature idea: uniquify outer SVG id #2

Closed mosesoak closed 2 years ago

mosesoak commented 2 years ago

Thank you for this excellent utility! Our team is using it with SVGO with the prefixIds plugin active.

The one problem that we still need to use the react hook here to solve is when we have multiple SVGs -- unfortunately SVGO's plugin can't easily uniquify those since it extends Webpack, which of course is just processing the assets, not the individual import cases.

This is an a11y concern, mostly, but passing that requirement is important to our company.

Example of how we currently do it, using this utility:

import { useUniqueInlineId } from '@inline-svg-unique-id/react';
import CheckmarkSVG from '~/images/icons/Checkmark.svg';

/** Uniquify id since there are more than one on the page */
const Checkmark: FC = () => {
  const checkmarkId = useUniqueInlineId();
  return <CheckmarkSVG id={checkmarkId} tw="mr-8" />;
};

Wondering how hard it would be to make this utility update each actual SVG element's id with unique counters? If it's a light lift, we'd be overjoyed if you added that feature! Thanks for considering it

laleksiunas commented 2 years ago

@mosesoak Can you elaborate on why is it necessary for you to have a unique id for each SVG icon? Are you targetting these SVG icons using ids?

mosesoak commented 2 years ago

Yes, it's due to a somewhat obscure Accessibility rule that comes up in site audits from certain tools. I think it's this one: https://www.w3.org/WAI/standards-guidelines/act/rules/id-value-unique-3ea0c8/

And I think it's possible that sometimes the IDs are referenced in aria tags as well.

However, if all we need to do is have a team convention of manually setting unique IDs for duplicate SVGs on a page, we can do that. In fact if Babel were setting the IDs then there wouldn't really be any way to target them in aria tags, so this feature request is probably not a good idea.

laleksiunas commented 2 years ago

Yes, if some Babel plugin is setting the ids, then you can't target anything. I suggest you try moving unique ids generation logic one layer up, something like this:

import React, { createContext, cloneElement, Children, useContext } from 'react';
import { useUniqueInlineId } from '@inline-svg-unique-id/react';

const sharedUniqueIdContext = createContext(undefined);

const SharedUniqueIdProvider = ({ children }) => (
  <sharedUniqueIdContext.Provider value={useUniqueInlineId()}>{children}</sharedUniqueIdContext.Provider>
);

const WithSharedId = ({ children }) => {
  const child = Children.only(children); // if you are using Typescript, you can strict type this component to accept only one child
  const sharedId = useContext(sharedUniqueIdContext);

  return cloneElement(child, { ...child.props, id: sharedId });
};

And use it:

const SomeComponent = () => (
  <SharedUniqueIdProvider>
    <span>Element targeting the svg</span> // create some component similar to WithSharedId to add aria prop
    <WithSharedId>
      <SomeSvgIcon />
    </WithSharedId>
  </SharedUniqueIdProvider>
);

In general, having some higher-order component wrapping Svg would reduce code duplication in your case - you won't need to wrap every svg in a separate component that uses useUniqueInlineId.

laleksiunas commented 2 years ago

Closing, feel free to reopen if needed.

mosesoak commented 2 years ago

Thank you for the reply @laleksiunas -- but no, I didn't mean that another Babel plugin was messing with ids. They are not being set. And I appreciate your suggestion of doing a HOC but your existing lib is already a good react-based solution.

Can you give me an idea of how hard this feature would be for you to develop, if it worked like this:

  1. detect SVG outer elements (I already see that in your code)
  2. look for an id on them, skip if found
  3. if no id is found, add a unique id

I am unfortunately not a babel plugin developer and although I can read your code I'm not sure exactly how to get it to inject ids on the outer elements in your system. Would you want to try it out and see if you could add those ids without much lift?

laleksiunas commented 2 years ago

@mosesoak Adding a unique id on the SVG element itself wouldn't be hard (it would work as you described), however, it would result in an unwanted bundle size increase and additional runtime checks for users that don't need this behavior (myself included). Why isn't it possible for you to ship icons without ID tags? The a11y concern that you attached in your issue description allows it.

This rule applies to any id attribute whose value is not an empty string ("")

mosesoak commented 2 years ago

Okay yeah can live without this, again probably easier (and more efficient as you pointed out) to manage this manually. Thanks for your conversation on it!