fix: pictogram rendering bugs

[vpicone]

Closes #4080

white boxes: We were both moving attributes to group elements while trying to take them away, this prevented us from adequately stripping/collapsing wrapper groups with more than one layer.

black boxes: we were only removing rectangles that matched the icon sizes: ['16', '20', '24', '32']. Since we're not gonna need bespoke pictogram sizes, extending this again shouldn't be necessary.

Preview link: https://carbon-pictograms.now.sh/guidelines/iconography/pictograms

[joshblack]

Left some questions above!

[vpicone]

@joshblack you're too quick, I answered both in the PR comment

[netlify]

Deploy preview for the-carbon-components ready!

Built with commit f750530

https://deploy-preview-4081--the-carbon-components.netlify.com

[netlify]

Deploy preview for carbon-elements ready!

Built with commit f750530

https://deploy-preview-4081--carbon-elements.netlify.com

[netlify]

Deploy preview for carbon-components-react ready!

Built with commit f750530

https://deploy-preview-4081--carbon-components-react.netlify.com

[vpicone]

@joshblack Also added a Pictogram component to the react build helpers so we don't get the Icon display name in dev tools. The components directory of icon-build helpers only had Icon, but needed some adjustments in builder.js to account for other components.

[vpicone]

There’s also 13 pictograms without categories. They’re under “undefined” at the preview link.

[joshblack]

Ignore this lol

[joshblack]

@vpicone my preference would be to treat all of them as Icon components under the hood if we could, is there any way that we could consolidate things under <Icon>? The only worry being that we're duplicating logic between them so naturally would have to update both any time an update came in, with this being the case might make sense for them to just be the same component.

[vpicone]

@joshblack ah I was thrown off by the components directory. My thought was they’re going to have a different height/width behavior as well as a different display name would warrant them having distinct components.

[joshblack]

What would be the difference for width/height? Wasn't sure off the top of my head 👀

[vpicone]

If we had unified size prop to the icon component that would restrict sizes to the intended set. That wouldn’t be relevant/included in a pictogram component.

[joshblack]

@vpicone wouldn't the case be that we just wouldn't pass in those props, or they would be undefined or bespoke, for the pictogram use-case? In other words:

// Icons
<Icon width={20} height={20} />
<Icon size="md" />

// Pictograms
<Icon width={64} height={48} />
<Icon />

[vpicone]

That size prop would be broken for pictograms. Using it would result in rendering smaller than they are intended to be shown. It also wouldn’t make sense having “lg” render smaller than the default size.

[joshblack]

@vpicone didn't mean for the prop values themselves to have any meaning really, just was trying to demonstrate that folks can pass them in or not and the underlying SVG would be fine. If we don't want folks to pass along a size prop then we don't forward that prop in the generated pictogram component 👍

[vpicone]

@joshblack but we’re still calling them icons? Should they have a different display name/prop types?

[joshblack]

@vpicone we could change the underlying name to SVG or something if you think it'd be less confusing, functionally these components seems to fill the same space so just was seeing if there was anything we could do to share the logic.

[chrisconnors-ibm]

@vpicone let me track down the undefined

[chrisconnors-ibm]

Across what attributes is search operating? It should be the same as icons scope and ordering.

[vpicone]

@chrisconnors-ibm it is, with the removal of subcategories.

[vpicone]

@joshblack what about the prop types? How would we differentiate that for icons vs pictograms

[joshblack]

@vpicone I had assumed with the generated components, what do you think? Icon was originally intended to render any SVG asset with the correct a11y props.

[vpicone]

@joshblack I think I just got confused with the component directory, I assumed it meant other components would go there not just Icon. I think yeah given Icons are a specific thing with different usage guidelines and props than Pictograms, we should rename the hardcoded Icon stuff to be something more inclusive of both components.

[chrisconnors-ibm]

(although I bet the two we're adding in #4041 in in are the only ones with aliases)

[vpicone]

@joshblack I removed the Pictogram component stuff, I think maybe I'm just misunderstanding how this abstraction is supposed to work. Opened #4090 to discuss further.

This now just fixes rendering bugs from the svgo optimization.