SVG Icons! #2028
SVG Icons! #2028
Conversation
revert documentalist usagePreview: documentation | landing | table |
|
I think I'm going to refactor to |
…d="string"). Icon component renders svg and title tags.
fix tree testPreview: documentation | landing | table |
remove style tag from endorsed icon (the only one that had it)Preview: documentation | landing | table |
| @@ -69,7 +69,7 @@ export class Alert extends AbstractPureComponent<IAlertProps, {}> { | |||
| return ( | |||
| <Dialog className={classNames(Classes.ALERT, className)} isOpen={isOpen} style={style}> | |||
| <div className={Classes.ALERT_BODY}> | |||
| <Icon iconName={iconName} iconSize="inherit" intent={Intent.DANGER} /> | |||
| <Icon iconName={iconName} width={40} intent={Intent.DANGER} /> | |||
There was a problem hiding this comment.
You never want to set width and height on an icon, only the size of its bounding box, so what about a size prop? I don't follow why iconSize has to be removed in the first place
There was a problem hiding this comment.
i think size is likely the way to go cuz we want them to be squared.
borrowed this pattern from researching other SVG icons, but it doesn't work super well.
| const filepath = path.resolve(__dirname, `../../resources/icons/16px/${icon.className}.svg`); | ||
| const svg = fs.readFileSync(filepath, "utf-8"); | ||
| const pathStrings = await svgo | ||
| .optimize(svg, { path: filepath }) |
|
What are the downsides of dropping support for the CSS API? |
| return ( | ||
| <svg className={classes} width={width} height={height} viewBox="0 0 16 16"> | ||
| <title>{iconName}</title> | ||
| {IconSvgPaths[shortName].map((d, i) => <path key={i} d={d} clip-rule="evenodd" fill-rule="evenodd" />)} |
There was a problem hiding this comment.
I was imagining that the consumer would feed this IconSvgPaths as a prop (iconSet?), so they can use this component with custom icon sets. Would also solve the 16px / 20px switch, since these two sizes are actually two different sets
There was a problem hiding this comment.
oh heck no, we want this to be as easy as possible for blueprint users. i think this Icon component should be the Blueprint icon component. if you want to use your own icons, you should do icon={<MyOwnIcon />} instead of icon="time".
There was a problem hiding this comment.
Oh I see, that wasn't clear to me, name feels very generic. Then maybe what you want is something like <Icon16 /> <Icon20 /> to reflect the icon set?
There was a problem hiding this comment.
What if the default behavior would choose 16 or 20 based on the given iconSize? like, the one that requires less scaling? just gotta figure out the math.
Dialog is also a generic name 😄
| const shortName = iconName.replace("pt-icon-", "") as IconName; | ||
| const classes = classNames(Classes.ICON, Classes.iconClass(iconName), Classes.intentClass(intent), className); | ||
| return ( | ||
| <svg className={classes} width={width} height={height} viewBox="0 0 16 16"> |
There was a problem hiding this comment.
shouldn't viewBox use the size/width props?
There was a problem hiding this comment.
no, viewBox is based on the size of the image itself, so it's either 16 or 20. width/height scale the viewBox.
|
@llorca main downside of dropping CSS support is that our CSS API sections won't be able to easily show icons (short of copy-pasting the SVG code every time). |
really awkward experience. makes me think we want to use 20 whenever iconSize > 20.
|
just pushed a commit 5e042e5 that's worth checking out:
|
fix testsPreview: documentation | landing | table |
.pt-label supports .pt-popover-wrapper
examples/core-examples/common/IconSelectPreview: documentation | landing | table |
| @@ -87,6 +87,14 @@ Styleguide pt-button | |||
| color: $pt-icon-color; | |||
| } | |||
|
|
|||
| // icon-only button | |||
| .pt-icon:first-child:last-child { | |||
| <Icon className={Classes.ALIGN_RIGHT} iconName={rightIconName} key="icon" />, | ||
| ]; | ||
| return ( | ||
| <> |
|
Monster work, killing it dude 🔥 Didn't QE everything yet, I'll need some more time. So far I have these comments:
that is 🔑, it is a reason why this PR exists in the first place 👍 |
| @@ -4,15 +4,62 @@ | |||
| See [**Icons**](#icons) for a searchable list of all available UI icons. | |||
| </div> | |||
|
|
|||
| <div class="pt-callout pt-intent-primary pt-icon-info-sign"> | |||
| <h5>SVG icons in 2.0</h5> | |||
| Blueprint 2.0 introduced SVG icon support throughout the ecosystem, and moved icon resources to a separate | |||
There was a problem hiding this comment.
nit: no need for the comma here (I noticed your prose style does this a lot and I generally don't mind for code comments, but I'm more picky about it in actual documentation).
There was a problem hiding this comment.
also, "throughout the ecosystem" is a bit of a stretch. I would remove that phrase entirely.
There was a problem hiding this comment.
sometimes, i use commas to indicate where to pause, for maximal dramatic effect. 🎭
| <h5>SVG icons in 2.0</h5> | ||
| Blueprint 2.0 introduced SVG icon support throughout the ecosystem, and moved icon resources to a separate | ||
| __@blueprintjs/icons__ package. The `Icon` component now renders SVG paths and the icon classes are no longer | ||
| used by any Blueprint React component. Icon font support has been preserved but should be considered legacy, |
There was a problem hiding this comment.
"... but should be considered a legacy feature that will be removed in a future major version."
| The component also accepts all valid HTML props for an `<svg>` element. | ||
|
|
||
| Data files in the __@blueprintjs/icons__ package provide SVG path information for Blueprint's 300+ icons | ||
| for 16px and 20px grids. The `iconName` prop dictates which SVG is rendered, and `iconSize` determines |
There was a problem hiding this comment.
another unnecessary comma here
There was a problem hiding this comment.
i actually thought long and hard about this one
| ```tsx | ||
| // string literals are supported through IconName union type | ||
| <Icon iconName="cross" /> | ||
| <Icon iconName="pt-icon-globe" iconSize={20} /> |
There was a problem hiding this comment.
probably shouldn't encourage the long names with pt- if you want to eventually remove them
There was a problem hiding this comment.
follow-up PR. still technically supported.
| @## CSS API | ||
|
|
||
| <div class="pt-callout pt-intent-warning pt-icon-warning-sign"> | ||
| <h5>Icon fonts are legacy in 2.0</h5> | ||
| Blueprint's icon fonts should be considered legacy, and we plan to remove them in a future major version. |
There was a problem hiding this comment.
use the same phrasing I suggested above
|
|
||
| public render() { | ||
| if (this.props.iconName == null) { | ||
| return null; | ||
| } | ||
| const { className, iconName, intent, iconSize = Icon.SIZE_STANDARD, ...restProps } = this.props; | ||
| const { className, iconName: iconNameProp, iconSize = Icon.SIZE_STANDARD, intent, ...svgProps } = this.props; | ||
| const iconName = iconNameProp.replace("pt-icon-", "") as IconName; |
There was a problem hiding this comment.
nit: leave the prop as iconName (also do this destructuring on the first line of the function, so you can use it on L46) and make this variable named normalizedIconName
|
Amazing stuff @giladgray! Couple of smaller issues:
|
separate PR please
can you elaborate? we'd still support it in the CSS API, right? does this mean all usages of |
|
@adidahiya yes, dropping long names precisely means that 👍 separate PR for |
|
ok, in that case I think we can make the break. An |
fix input-group button icon sizePreview: documentation | landing | table |
* Callout renders SVG Icon component - added .pt-callout-icon class to adjust padding & layout when icon is present - existing CSS API is unchanged - add Callout Example in docs * fix tests * 👏 totally non-breaking API! only one use of .pt-callout-icon * iconName=null overrides intent
Callout renders SVG Icon component (#2060)Preview: documentation | landing | table |
clarify callout icon logicPreview: documentation | landing | table |
|
things that still use the icon font:
other bugs:
|
Fixes #365
Checklist
Calloutstill uses icon classes because it relies on::beforeandpositiontrickerycan't resolve this without an API breaktotally did it Callout renders SVG Icon component #2060MenuItemicon colorChanges proposed in this pull request:
generate-icons-sourcescript now generatesiconSvgPaths.tsxfile withexport const IconSvgPaths{16,20}: Record<IconName, string[]>where value is array of<path d="..." />stringsIconNameonly includes short names.LegacyIconNameextends it with long names.Recordtype above to support indexingIconcomponent now renders SVG content instead ofspan.pt-icon...iconSizeprop, replaced withwidth/height. this isn't totally ideal, I could use some help figuring out the ideal API here.Reviewers should focus on:
Follow-up work
iconName?: IconNameprop ⇒icon?: IconName | JSX.Element? drop support for long icon names in iconName prop #2067IconNameorLegacyIconNameiconName ⇒ icon, accept JSX #2066pt-icon-time?<Icon iconSize={20} ... />in that case