-
Notifications
You must be signed in to change notification settings - Fork 55
Conversation
Codecov Report
@@ Coverage Diff @@
## master #50 +/- ##
==========================================
- Coverage 87.36% 86.91% -0.46%
==========================================
Files 74 75 +1
Lines 1148 1169 +21
Branches 217 214 -3
==========================================
+ Hits 1003 1016 +13
- Misses 141 147 +6
- Partials 4 6 +2
Continue to review full report at Codecov.
|
|
||
const svgIcons: { [name: string]: ISvgIconSpec } = { | ||
umbrella: { | ||
viewBox: '0 0 34 34', |
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.
Should we have a default value for the viewBox? In Teams, not all, but most of the icons have the same viewBox which is '0 0 32 32'
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.
it should be defined for corresponding module with Teams icons then - we should not introduce any Teams-specific aspects to common logic of Stardust library (also, the 'umbrella' icon is introduced here for demo purposes only).
Here is a way I see this module could be organised to contain Teams SVG icons bank:
// teams-icons.jsx
const iconTemplates = {
"teamsCreate": (<path d="..." />),
"startCall": (
<React.Fragment>
<path d="" />
<path d="" />
</React.Fragment>)
....
}
const commonViewBox = "0 0 32 32"
let iconsToExport = {}
// 'reduce' will suit better these needs, but idea is the same
Object.keys(iconTemplates)
.forEach(iconName => {
iconsToExport[iconName] = {
element: iconTemplates[iconName],
viewBox: commonViewBox
}
})
export default iconsToExport
Does this support more complex SVGs, like SVGs with more paths? Like,
|
umbrella: { | ||
viewBox: '0 0 34 34', | ||
element: ( | ||
<path d="M27 14h5c0-1.105-1.119-2-2.5-2s-2.5 0.895-2.5 2v0zM27 14c0-1.105-1.119-2-2.5-2s-2.5 0.895-2.5 2c0-1.105-1.119-2-2.5-2s-2.5 0.895-2.5 2v0 14c0 1.112-0.895 2-2 2-1.112 0-2-0.896-2-2.001v-1.494c0-0.291 0.224-0.505 0.5-0.505 0.268 0 0.5 0.226 0.5 0.505v1.505c0 0.547 0.444 0.991 1 0.991 0.552 0 1-0.451 1-0.991v-14.009c0-1.105-1.119-2-2.5-2s-2.5 0.895-2.5 2c0-1.105-1.119-2-2.5-2s-2.5 0.895-2.5 2c0-1.105-1.119-2-2.5-2s-2.5 0.895-2.5 2c0-5.415 6.671-9.825 15-9.995v-1.506c0-0.283 0.224-0.499 0.5-0.499 0.268 0 0.5 0.224 0.5 0.499v1.506c8.329 0.17 15 4.58 15 9.995h-5z" /> |
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.
A lot of SVGs can get really long and be pretty complex. I wonder if we might want to consider not inlining them like this in the code as I think it could get really hard to scan and edit. In our project today, an SVG has its own html file which gives the added benefit of formatting
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, there multiple ways to embrace this use case - essentially, there is no difference for the common Icon's logic on where and how the child content will be provided. There are several HTML-to-JSX transpilers that could be introduced as part of the build logic - those will consume HTML templates as input and create corresponding JS modules for each of the icons. Here is one of them: https://magic.reactjs.net/htmltojsx.htm
At the same time, the same could be achieved much simplier - if I've understood you correctly, the situation is that there is a dedicated HTML template for each SVG icon. In that case svgIcons
module could be organised in the following way:
// teamCreateIcon.tsx - provides 'teams-create' icon, only
const teamCreateIcon = (
// HTML template goes here - essentially, the same syntax
<g>
<path d="..."/>
...
</g>
)
export default teamCreateIcon
// index.js - just assembling all the definitions from multiple files
export { default as TeamCreateIcon } from './teamCreateIcon'
export { default as StartCallIcon } from './startCallIcon'
Returning to aforementioned question if it is possible to support scenarios where multiple path objects will be provided (instead of one) - sure, it is. Just use React.Fragment
as a parent element for this:
// teamCreateIcon.tsx - provides 'teams-create' icon, only
const teamCreateIcon = (
// HTML template goes here - essentially, the same syntax
<React.Fragment>
<path d="..." />
<path d="..." />
...
</React.Fragment>
)
export default teamCreateIcon
|
||
return ( | ||
<ElementType className={classes.root} {...rest}> | ||
<svg className={classes.svg} viewBox={icon && icon.viewBox}> |
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.
should we allow optional accessibility props here?
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, definitely - this will go next as part of dedicated PR
'name', | ||
'size', | ||
'xSpacing', | ||
] | ||
|
||
static defaultProps = { | ||
as: 'i', | ||
kind: 'FontAwesome', | ||
as: 'span', |
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.
Is there a reason you're changing this from the I tag? I is (or was) standard, for the most part.
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.
from MDN
The HTML element represents a range of text that is set off from the normal text for some reason. Some examples include technical terms, foreign language phrases, or fictional character thoughts. It is typically displayed in italic type.
<span>
seems to be a more appropriate and neutral element, especially given that we have SVG-based icon as a default option.
src/components/Icon/iconRules.ts
Outdated
@@ -35,7 +36,35 @@ const getIcon = (kind, name) => { | |||
return { content, fontFamily } | |||
} | |||
|
|||
const getSize = size => `${sizes.get(size)}em` || '1em' | |||
const getSize = (size, isFontBased) => | |||
isFontBased ? `${sizes.get(size)}em` : `${sizes.get(size)}rem` |
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.
Why ems for fonts and rems for non-fonts?
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.
this is a good observation - it seems that we could use the same principle of relative scaling for both. Will address this
@@ -3,20 +3,10 @@ import { Icon } from '@stardust-ui/react' | |||
|
|||
const IconExample = () => ( | |||
<div> | |||
<Icon size="big" name="chess rock" /> |
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.
rook - not rock ;)
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.
:) Thank you!
<Icon disabled name="umbrella" size="big" /> | ||
<Icon disabled name="umbrella" size="big" variables={{ color: 'blue' }} /> | ||
<Icon disabled name="umbrella" size="big" variables={{ color: 'red' }} /> | ||
<Icon disabled name="umbrella" size="big" variables={{ color: 'orange' }} /> |
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.
We should update this branch with the latest master and try to fixed the other components that use icons, as well as their examples, because currently those are not working.
<Icon font="FontAwesome" name="calendar alternate outline" bordered /> | ||
<Icon font="FontAwesome" name="coffee" bordered /> | ||
<Icon font="FontAwesome" name="compass outline" bordered /> | ||
<Icon font="FontAwesome" name="area chart" bordered /> |
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.
Are these still necessary?
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.
should be removed, thanks!
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.
If all other components are consistent with the previous look, I would merge it.
Icon
SVG-based icons are introduced as a default mechanism for creating icon. Font-based icons are still available to the client, but font-based implementation should be explicitly requested:
TODO
API Proposal
Name
By default icon from fonts collection is rendered (FontAwesome, Icons).
with HTML:
Font
If font is provided, an icon with the
name
is rendered from the collection of glyphs for corresponding font.SVG
Collection of SVG icons is provided in a separate module, each icon item contains all the necessary DOM to be rendered as child of
<svg>
element.To render SVG icon
svg
property should be set for theIcon
component - withname
defining the icon to render from SVG icons' bank:results in the following HTML: