Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

feat(icon): color prop #651

Merged
merged 4 commits into from
Dec 21, 2018
Merged

feat(icon): color prop #651

merged 4 commits into from
Dec 21, 2018

Conversation

bmdalex
Copy link
Collaborator

@bmdalex bmdalex commented Dec 19, 2018

feat(icon): color prop

Description

This PR:

  • adds color prop to Icon component
  • creates Icon color examples

API

<Icon color={COLOR} .../>

where COLOR is one of 'primary' | 'secondary' | 'blue' | 'green' | 'grey' | 'orange' | 'pink' | 'purple' | 'teal' | 'red' | 'yellow' | string

screenshot 2018-12-19 at 19 38 04

Copy link
Member

@layershifter layershifter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, left some comments 👍

@bmdalex bmdalex force-pushed the feat/icon-color-prop branch 2 times, most recently from 80558d3 to 0df0229 Compare December 20, 2018 13:41
Copy link
Contributor

@kuzhelov kuzhelov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets please take another look on removed extensibility point for IconVariables interface

<div>
<ProviderConsumer
render={({ siteVariables: { emphasisColors, naturalColors } }) =>
_.take(_.keys({ ...emphasisColors, ...naturalColors }), 3).map(color => (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this expression is very confusing - there should be a way to simplify this example. Actually, why slicing is necessary here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

screenshot 2018-12-19 at 19 38 04
just to make the example fit nicely, any suggestions for improving, @kuzhelov ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kuzhelov can I resolve this?

src/components/Icon/Icon.tsx Show resolved Hide resolved
@kuzhelov kuzhelov added needs author feedback Author's opinion is asked and removed 🚀 ready for review labels Dec 20, 2018
@bmdalex bmdalex added 🚀 ready for review and removed needs author feedback Author's opinion is asked labels Dec 20, 2018
@bmdalex bmdalex force-pushed the feat/icon-color-prop branch 2 times, most recently from 7984a49 to 16d9f69 Compare December 21, 2018 11:58
@bmdalex bmdalex merged commit bcff4a5 into master Dec 21, 2018
@bmdalex bmdalex deleted the feat/icon-color-prop branch December 21, 2018 15:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants