-
Notifications
You must be signed in to change notification settings - Fork 347
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
Icon: Add Compact Icons #3771
Icon: Add Compact Icons #3771
Conversation
✅ Deploy Preview for gestalt ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
They should be 24-24 and we can adjust size internally. This should be just moving them out to an internal folder and removing them from the index, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Run yarn tsc please before merging. LGTM
@@ -0,0 +1,5 @@ | |||
{ | |||
"rules": { | |||
"max-len": 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just curious, what does this rule do? (in the context of this PR)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* | ||
* See the [size](https://gestalt.pinterest.systems/web/icon#Size) variant to learn more. | ||
*/ | ||
size?: number | string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't we limit size here?, otherwise we are on the same place
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
they should be '16' the only accepted value for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But maybe you are right in that they should be fixed - I guess we should force them to be 16x16 to limit usage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
| 'light' | ||
| 'dark'; | ||
|
||
type IconName = keyof typeof icons | keyof typeof compactIconsVR | keyof typeof compactIconsClassic; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type IconName = keyof typeof icons | keyof typeof compactIconsVR | keyof typeof compactIconsClassic;
do we need : | keyof typeof compactIconsClassic;
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess all VR icons will contain classic right? So maybe you are correct that we don't need this
@@ -222,6 +223,20 @@ Should be used sparingly and only in places where the UI is very dense and a lar | |||
title="Custom SVG icon" | |||
/> | |||
</MainSection.Subsection> | |||
<MainSection.Subsection |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we do a follow up PR adding a single page for IconCompact?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes - I think we should. I wanted to wait since there's not too many right now. and it's a replica of Icon
Pull Request Instructions
Init add of new compact icons.
This also moves all icons to a new
InternalIcon
that supports component icons. You can now use these in the badge component to consume the icons at 16x16 ratios as needed.There is a new public
IconCompact
that can also be used for compact iconsChecklist