Icon instance className overwrites context className

[killface]

If I specify a className in my context provider, eg.

    <IconContext.Provider
      value={{ className: 'icon' }}
      ...
   />

And then specify a className when I use my icon, eg. <CalendarCheck className="foo" />,
... then my icon only gets the foo className and I can no longer target the element with the .icon selector.

I'd propose bringing in a small library like clsx and updating IconBase to merge the two className values on the <svg> element, eg. className={clsx(contextClassName, className)}.

I'd be happy to create a PR with the proposed changes, given a bit of guidance on how to contribute to this project (and an okay for adding a third-party dependency).

[rektdeckard]

AFAIC this is the expected behavior in React -- passing explicit props always overrides, not adds. The same is true in vanilla JS with object spread operator, etc. I also do not wish to add any external runtime dependencies to this lib, so I suggest just using a context consumer to get your context className and combine it yourself:

<IconContext.Provider value={{ className: "icon" }}>
  <IconContext.Consumer>
    {({ className }) => (<Cube className={clsx(className, "foo")} />)}
  </IconContext.Consumer>
<IconContext.Provider>

And since these are simple strings, you can really just concatenate them className={className + " foo"}.

[killface]

Oh, interesting! I'm somewhat new to React and wasn't aware of the context Consumer. I'll look into that route - is there a way to make it generic so ALL Phosphor Icons get the classNames merged/appended, or will I need to use an <IconContext.Consumer around each icon instance where I want this functionality?

Thanks for the prompt response, btw!

[rektdeckard]

Sure thing! It isn't possible to get this behavior implicitly, but you can write a simple wrapper to make it easy to reuse without having to type out the consumer every time:

import clsx from 'clsx';
import { Icon, IconContext, IconProps, Cube } from '@phosphor-icons/react';

const WithCtxClass = (props: IconProps & { icon: Icon }) => {
  const { icon: I, ...rest } = props;
  return (
    <IconContext.Consumer>
      {/* Important that the rest props come first, otherwise prop className will override merged classNames */}
      {({ className }) => (<I {...rest} className={clsx(className, props.className)} />)}
    </IconContext.Consumer>
  );
};

const App = () => {
  return (
    <IconContext.Provider value={{ size: 48, className: "icon" }}>
      <WithCtxClass icon={Cube} className="foo" />
    </IconContext.Provider>
  );
};

Or, you can make a factory function to prepare the wrapped icons beforehand:

function withCtxClass(icon: Icon) {
  const I = icon;
  return (props: IconProps) =>  {
    return (
      <IconContext.Consumer>
        {({ className }) => (<I {...props} className={clsx(className, props.className)} />)}
      </IconContext.Consumer>
    );
  }
}

const CombinedCube = withCtxClass(Cube);

const App = () => {
  return (
    <IconContext.Provider value={{ size: 48, className: "icon" }}>
      <CombinedCube className="foo" />
    </IconContext.Provider>
  );
};

[rektdeckard]

Working demo

[killface]

I really appreciate the examples! It still feels a lot of effort just to add a class to every icon.

I hear what you're saying about overwriting props being the default/expected behavior in React and JS, but I don't believe it's uncommon to treat a prop like className differently. I feel like this fits a little better with CSS's tendency toward "inheritance", and practically-speaking is more often what an engineer probably wants.

I think your argument for simply concatenating the two classNames is perfectly reasonable, too. What about something like:

className={[contextClassName, className].filter(Boolean).join(" ")}

or

  const theSpaceBetween = contextClassName && className ? " " : "";
  <svg
     className={contextClassName + theSpaceBetween + className}
     ...

I suppose changing this behavior could possibly break someone else's current implementation if they're relying on the instance className to overwrite the context className. It seems unlikely, but to be safe I suppose this might have to be opt-in.

Alternatively, what if you just added a class like ph or phosphor to every icon by default and we could use that?

[rektdeckard]

I totally appreciate your need here. All the same, what you're proposing is unfortunately not really "reactish". It more closely resembles inheritance rather than composition. A good counterexample is to ask how would someone omit the context class, if they needed to in a specific case? It wouldn't be possible. That indicates a flaw in API design, in my opinion. Plus, as a popular library, we will always try our best not to break anyone's applications, ever. And given our number of users, someone is surely relying on the existing behavior, somewhere.

Your question pertains to CSS, which by definition, grants inheritance. If you don't like a React-based wrapper solution, why not simply use CSS as it was intended, to target the elements you need?

<div className="something">
  <div>
    <div>
      <Cube className="icon" />
    </div>
  </div>
</div>

.something svg.icon {
 transform: rotate(45deg);
}

[rektdeckard]

Or you could simply...you know...add the class to the element?

FWIW, I do believe it is extremely unexpected for className, or any other intrinsic HTMLAttribute prop, to have its behavior hijacked and modified. Our icons are specifically designed to be transparent, meaning a <Cube /> exposes all of the underlying props/attributes of an <svg />, including being forwardRefs. Behavior is only added, never changed.

[killface]

Alas, I was really hoping for a simple way to apply a single class to every Phosphor icon. Seems that's not in the cards. Thanks for your time.