Skip to content

Commit

Permalink
feat(icon): upgrade props
Browse files Browse the repository at this point in the history 
- remove classnames lib dependency
- add inverted variant
- add three sizes
- deprecate fixedWidth
- migrate CSS to a module
  • Loading branch information
@theetrain
theetrain committed Sep 18, 2017
1 parent 24e5c82 commit 8720121
Show file tree
Hide file tree
Showing 6 changed files with 348 additions and 131 deletions.
9 changes: 8 additions & 1 deletion src/components/Input/__tests__/__snapshots__/Input.spec.jsx.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,14 @@ exports[`Input renders with a feedback state and icon 1`] = `
>
<i
aria-hidden="true"
class="icon icon-core-exclamation-point-circle"
class="
icon
icon-core-exclamation-point-circle
icon-color--inherit
icon--medium
"
/>
</div>
</div>
Expand Down
35 changes: 19 additions & 16 deletions src/old-components/Icon/Icon.jsx
View file
Original file line number Diff line number Diff line change
@@ -1,18 +1,16 @@
import React from 'react'
import PropTypes from 'prop-types'
import classNames from 'classnames'
import styles from './Icon.modules.scss'

const Icon = ({ glyph, variant, fixedWidth, size, className, children, ...rest }) => {
const classes = classNames(
'icon',
`icon-core-${glyph}`,
className,
{
'icon--fw': fixedWidth,
[`icon--${variant}`]: variant,
'icon--large': size
}
)
const classes = `
${styles.icon}
${styles[`icon-core-${glyph}`]}
${variant ? styles[`icon-color--${variant}`] : styles['icon-color--inherit']}
${fixedWidth ? styles['icon--fw'] : ''}
${size ? styles[`icon--${size}`] : ''}
${className}
`

return (
<i {...rest} className={classes}>
Expand Down Expand Up @@ -44,21 +42,26 @@ Icon.propTypes = {
]).isRequired,
/**
* The appearance of the Icon.
*
* **Note**: when no variant is specified, the icon's colour is inherited from parent element.
*/
variant: PropTypes.oneOf([
'primary',
'secondary',
'disabled',
'inverted',
'error'
]),
/**
* Whether or not to give the icon a fixed width.
*
* @deprecated please use [Unordered Lists](#unorderedlist) to

This comment has been minimized.

Sign in to view

Copy link
@lzcabrera

lzcabrera Sep 18, 2017

Member

Note: Currently UnorderedList doesn't have the ability to display different icons within the same list.
* display icons uniformly within a column.
*/
fixedWidth: PropTypes.bool,
/**
* @ignore
*
*/
size: PropTypes.oneOf(['large']),
size: PropTypes.oneOf(['small', 'medium', 'large']),
/**
* One or more CSS class names separated by spaces to append onto the icon.
* Don't advertise as we plan on removing this feature soon.
Expand All @@ -75,9 +78,9 @@ Icon.propTypes = {
children: PropTypes.node
}
Icon.defaultProps = {
variant: null,
variant: undefined,
fixedWidth: false,
size: null,
size: 'medium',

This comment has been minimized.

Sign in to view

Copy link
@lzcabrera

lzcabrera Sep 19, 2017

Member

I feel like default should be 1rem (which in this case happens to be small)
Because it is the one size with the most use cases (chevron link and forms)

This comment has been minimized.

Sign in to view

Copy link
@malikalim1

malikalim1 Sep 19, 2017

Contributor

Doesn't 1rem = 16px? Thought that was medium

This comment has been minimized.

Sign in to view

Copy link
@lzcabrera

lzcabrera Sep 19, 2017 •
edited
Loading

Member

Correct, hence the reason for my jira comment on TDS-92 re: size prop options.

http://localhost:6060/#chevronlink
className: '',
children: null
}
Expand Down
90 changes: 90 additions & 0 deletions src/old-components/Icon/Icon.modules.scss
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
@import '../../scss/settings/icons';
@import '../../scss/settings/colours';
@import '../../scss/utility/icons-utility';

@font-face {
font-family: "TELUS Core Icons";
src: url('#{$icon-font-prefix}/core-icons.eot');
src:
url('#{$icon-font-prefix}/core-icons.woff2') format('woff2'),
url('#{$icon-font-prefix}/core-icons.woff') format('woff'),
url('#{$icon-font-prefix}/core-icons.ttf') format('truetype'),
url('#{$icon-font-prefix}/core-icons.eot?#iefix') format('eot');
font-weight: normal;
font-style: normal;
}

This comment has been minimized.

Sign in to view

Copy link
@ryanoglesby08

ryanoglesby08 Sep 20, 2017 •
edited
Loading

Contributor

We probably don't want to put the @font-face rule for the icon font into the CSS modules file since it is a global property.

It is already defined in "src/scss/foundation/_core-icons.scss" so no need to declare it twice. We can always move the @font-face rule when we remove the old CSS for icons (which is a later story).

This comment has been minimized.

Sign in to view

Copy link
@lzcabrera

lzcabrera Sep 20, 2017

Member

I was the one who suggested to move it out of _core-icons.sss because we should be able to remove that file and not break the icon component. Which could happen before we get to migrate to SVGs.

I was going to argue that this font-face is only used on the Icon component, but I realized yesterday that form checkboxes also use the font icon.

For that reason, I agree it shouldn't live here. Perhaps we can move it foundation/_fonts.scss when we are ready to kill foundation/_core-icons.scss.

This comment has been minimized.

Sign in to view

Copy link
@theetrain

theetrain Sep 21, 2017

Author Contributor

I can't believe there's a GitHub user named @font-face.

.icon {
@include core-icon;

transition: color .1s linear;

&--primary {
color: $color-icon-primary;
}

&--secondary {
color: $color-icon-secondary;
}

&--error {
color: $color-cardinal;
}

&--disabled {
color: $color-icon-disabled;
}

&--fw {
width: 1.09rem;
text-align: center;
}

&--small {
font-size: 1 rem;
}

&--medium {
font-size: 1.5rem;
}

&--large {
font-size: 3rem;
}
}

@each $name, $codepoint in $core-icon-codepoints {
.icon-core-#{$name}::before {
content: $codepoint;
}
}

// Variants
.icon-color--inherit {
color: inherit;
}

.icon-color--primary {
color: $color-primary;
}

.icon-color--secondary {
color: $color-secondary;
}

.icon-color--inverted {
color: $color-white;
}

.icon-color--error {
color: $color-icon-error;
}

.icon-core-left-chevron {
@include core-icon(chevron);

&::before {
display: inline-block;
transform: rotate(-180deg) translateY(1.5px);
}
}

This comment has been minimized.

Sign in to view

Copy link
@ryanoglesby08

ryanoglesby08 Sep 20, 2017

Contributor

Let's switch these class names to CSS Modules convention which is camelCase, not snake-case.

This comment has been minimized.

Sign in to view

Copy link
@ryanoglesby08

ryanoglesby08 Sep 20, 2017

Contributor

Let's use more conventional CSS modules semantics here instead of BEM style.

So instead of icon and icon--primary we should have a base icon class that gets composed into other classes (if appropriate). Otherwise, we should not have any nesting or BEM syntax.

// Example

.icon {
  @include core-icon; // or just inline this mixin as it is now deprecated
}

.primary {
  composes: icon;

  color: $color-icon-primary;
}

This comment has been minimized.

Sign in to view

Copy link
@theetrain

theetrain Sep 20, 2017

Author Contributor

These were deleted.

This comment has been minimized.

Sign in to view

Copy link
@ryanoglesby08

ryanoglesby08 Sep 20, 2017

Contributor

Hmmmmm I see that the classes have been changed a bit, but my comment still seems applicable...
6 changes: 3 additions & 3 deletions src/old-components/Icon/__tests__/Icon.spec.jsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ describe('<Icon />', () => {
it('supports variants', () => {
const icon = doShallow({ variant: 'secondary' })

expect(icon).toHaveClassName('icon--secondary')
expect(icon).toHaveClassName('icon-color--secondary')
})

it('can be fixed width', () => {
Expand All @@ -35,9 +35,9 @@ describe('<Icon />', () => {
})

it('can be sized', () => {
const icon = doShallow({ size: 'large' })
const icon = doShallow({ size: 'small' })

expect(icon).toHaveClassName('icon--large')
expect(icon).toHaveClassName('icon--small')
})

it('supports custom CSS classes', () => {
Expand Down
9 changes: 8 additions & 1 deletion src/old-components/Icon/__tests__/__snapshots__/Icon.spec.jsx.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,13 @@

exports[`<Icon /> renders 1`] = `
<i
className="icon icon-core-checkmark"
className="
icon
icon-core-checkmark
icon-color--inherit
icon--medium
"
/>
`;
Loading

0 comments on commit 8720121

Please sign in to comment.