primer / view_components

ViewComponents for the Primer Design System
https://primer.style/components/
MIT License
449 stars 115 forks source link

Icon: provide support for `large` and `xlarge` sizes #2358

Closed francisfuzz closed 9 months ago

francisfuzz commented 10 months ago

Summary

At this time of writing Primer Icon allows for the following sizes:

One of :xsmall (12), :small (16), or :medium (24).

It would be nice to have support for large and xlarge sizes. 🙏

Use case: within my own work at GitHub, I'm working on a redesign that could benefit from using those larger sizes. However, the absence of these sizes isn't a showstopper -- as a work-around, I'm leveraging some helpers to get the same thing done. 😉

francisfuzz commented 10 months ago

If contributions from Hubbers outside of the Primer team are welcome, I'm interested and would love to figure out next steps!

🙇 ✨

lesliecdubs commented 10 months ago

Thanks for the suggestion @francisfuzz! We should get confirmation from Primer Design on the appropriate size options for the Primer Icon before we move forward, so I'm adding this to their queue to get that perspective. I'm temporarily removing the rails label to clear this issue from the engineering inbox while we await confirmation.

If you're able to share a link/image/Figma/example of the UI you're working on that would benefit from these larger sizes, I imagine that would also be useful 🙏

francisfuzz commented 10 months ago

If you're able to share a link/image/Figma/example of the UI you're working on that would benefit from these larger sizes, I imagine that would also be useful 🙏

@lesliecdubs - Thanks for considering my ask! 🙇

Sure thing -- I've shared a header preview for what I'm working on below, where the GitHub icon is sized up to 32 x 32 pixels using one of our utility methods rather than using the Primer Icon.

Header preview for GitHub Education

tallys commented 9 months ago

looping in @camertron and @gavinmn - thoughts on this proposal? also @joseph-lozano could be an octovisuals opportunity

camertron commented 9 months ago

This proposal seems mostly fine to me 😄 My only concern is that some of our icons are not designed to be displayed at larger or smaller sizes. For example, we have 12px, 16px, and 24px versions of the alert icon so that the exclamation point in the center is properly readable at those various sizes. I assume it would be ok to scale up the 24px versions to 32px and larger, but I would need to double-check that assumption with our design folks first.

mperrotti commented 9 months ago

Also, it looks like octicons-react sizes are out of sync with the Primer ViewComponents sizes you posted in the description:

const sizeMap = {
  small: 16,
  medium: 32,
  large: 64
}

(from lib/octicons_react/src/createIconComponent.js)

I'm not sure what the "right" values are, but we should align on a consistent set of icon sizes and add them to Primer Primitives. What do you think @langermank and @lukasoppermann ?

camertron commented 9 months ago

@mperrotti oh good find! The octicons themselves are available at 12px, 16px, and 24px, so I would think those are the appropriate sizes to offer in octicons-react.

mperrotti commented 9 months ago

It's also worth noting that we accept arbitrary number values for the icon size in React: https://github.com/primer/octicons/blob/6bc78de32be1a218a69e10a731072331ba94e7f9/lib/octicons_react/src/createIconComponent.js#L28C39-L28C43

This makes the icon component more flexible, but opens us up to inconsistencies. I actually think this is a helpful "escape hatch", but I'm open to keeping icon sizes strict if other designers think that's necessary.

camertron commented 9 months ago

I wonder if we could offer standard sizes and custom size values, preferring standard sizes in most cases?

lukasoppermann commented 9 months ago

I think since we have actual assets, adding primitives is not necessary.

I agree with @camertron that our icons are not design to be displayed at larger sizes. They may look very bad in e.g. 64px as the line thickness will be too big and larger icons typically have more details to not look odd.

If we add the sizes we suggest that they are fine to use at larger sizes and I think they are probably not.

I don't know about rails but in react a user can just overwrite the size with custom css, right? This feels better to me as it is an obvious customization "at your own risk".

tallys commented 9 months ago

Agree with @lukasoppermann - let's do this as a one-off and not change the library, especially since this is also our logomark. Octicions will not be the home for logos and brand assets.