feat(icons): make icons themeable (breaking change) #726

montezume · 2019-05-02T17:28:58Z

Summary

As we discussed on slack, in order to make the icons themeable, we need to do something about the theme prop. When you use a styled component in emotion, it injects its theme prop, which we were overwriting since we had an identically named prop.

💣 BREAKING CHANGES 💣

Renames the prop theme to color. Renames it's possible values from being our old colour decisions (white, green, etc) to our new colour decisions (surface, primary, etc).

Why do we need themeable icons?

Components that use icons, like for example, the TimeInput need this to maintain consistency.

Why are you spelling colour wrong?

😭

This will be merged into the 10.0.0 branch. The PR is set to master now so that Percy runs.

emmenko

LGTM! 💯

PS: there are some weird inconsistencies in percy...or did we break something?

emmenko · 2019-05-02T19:20:56Z

src/components/field-label/README.md

@@ -39,7 +39,7 @@ The `hintIcon` also accepts a custom `theme` while defaulting to `orange` in the
  onInfoButtonClick={() => {}} />}
  hint={<FormattedMessage {...messages.hint} />}
 - hintIcon={<WarningIcon />}
-+ hintIcon={<WarningIcon theme="green" />}
+ hintIcon={<WarningIcon color="green" />}


Suggested change

+ hintIcon={<WarningIcon color="green" />}

+ hintIcon={<WarningIcon color="primary" />}

emmenko · 2019-05-02T19:21:48Z

src/components/icons/README.md

+| Props   | Type     | Required | Values                                                        | Default | Description                                                                                            |
+| ------- | -------- | :------: | ------------------------------------------------------------- | ------- | ------------------------------------------------------------------------------------------------------ |
+| `size`  | `string` |          | 'small', 'medium', 'big', 'scale'                             | 'big'   | Specifies the icon size (if `scale` is selected, the dimensions will scale according with the parents) |
+| `color` | `string` |          | 'solid', 'neutral60', 'info', 'primary40', 'warning', 'error' | 'solid' | Specifies the icon color                                                                               |


tdeekens · 2019-05-02T20:02:48Z

src/components/icons/create-styled-icon.js

+    case 'warning':
+      return overwrittenVars.colorWarning;
+    case 'error':
+      return overwrittenVars.colorError;


Wondering if this could be more of a convention approach instead of switching?

Still valid?

can you check the latest version?

tdeekens

Wondering if we should have this to a base branch of release/x.x.x if we want more accumulate more breaking changes.

tdeekens · 2019-05-02T20:03:19Z

src/components/panels/collapsible-panel/header-icon.js

-  if (isDisabled) return 'grey';
-  if (tone === 'urgent') return 'white';
-  return 'black';
+const getArrowColor = ({ tone, isDisabled }) => {


Good rename!

montezume · 2019-05-02T20:55:29Z

I think the Percy diff it's a combination of breaking things and of the icon VRTs (7 I think) have different labels so therefore have a diff. But yeah, must be some broken things. I'll fix up the broken things tomorrow 😄

montezume · 2019-05-02T20:55:52Z

@tdeekens I think that a v10 branch is a really good idea 👍

montezume · 2019-05-03T08:15:28Z

Ah Percy doesn't run if I have it aimed against a different branch. I guess that makes sense.

montezume · 2019-05-03T08:35:16Z

src/components/icons/create-styled-icon.js

+    ...theme,
+  };
+
+  const iconColor = overwrittenVars[`color${capitalize(color)}`];


is this more like what you were thinking @tdeekens ?

Ah yeah. Thanks!

aminbenselim

it's funny because the old values were more color than the new ones 😅

aminbenselim · 2019-05-03T08:52:35Z

src/components/buttons/icon-button/icon-button.js

    }
-    return 'white';
+    return 'surface';
  }

  // if button is disabled, icon should be grey


should we update the comments too ?

montezume · 2019-05-03T09:26:14Z

Okay, I'll merge this into 10.0.0 then. Thanks 👍

* feat(icons): make icons themeable (breaking change)

feat(icons): make icons themeable (breaking change)

ff97f82

montezume self-assigned this May 2, 2019

montezume requested review from emmenko, tdeekens and pa3 May 2, 2019 17:29

montezume added Semver: BREAKING 💥 Type: Breaking Change labels May 2, 2019

montezume added 8 commits May 2, 2019 19:32

chore: fix vspec

896c26e

fix: time input use correct icon theme

9197808

chore: rename prop

07dae34

chore: refactor rest of icon usage to use correct prop

bcf8747

fix: rename theme -> color

e1ca805

chore: more renaming

36fca3a

chore: another one

f657ad8

chore: another one

8e58bdd

emmenko approved these changes May 2, 2019

View reviewed changes

tdeekens reviewed May 2, 2019

View reviewed changes

montezume changed the base branch from master to 10.0.0 May 3, 2019 07:50

chore: more fixes

34f0984

montezume changed the base branch from 10.0.0 to master May 3, 2019 08:15

montezume added 3 commits May 3, 2019 10:16

fix(field-label): icon refactoring

8e65eea

chore: money input icon fix

373f750

chore: pr fixes

ef653e9

montezume commented May 3, 2019

View reviewed changes

montezume requested a review from aminbenselim May 3, 2019 08:42

aminbenselim approved these changes May 3, 2019

View reviewed changes

chore: update comment

601a676

montezume changed the base branch from master to 10.0.0 May 3, 2019 09:17

tdeekens approved these changes May 3, 2019

View reviewed changes

montezume merged commit cc8653e into 10.0.0 May 3, 2019

montezume deleted the ml-icon-theme branch May 3, 2019 09:26

montezume mentioned this pull request May 3, 2019

Icons should support theming #725

Closed

montezume added a commit that referenced this pull request May 6, 2019

feat(icons): make icons themeable (breaking change) (#726)

7b7ba83

* feat(icons): make icons themeable (breaking change)

montezume added a commit that referenced this pull request May 6, 2019

feat(icons): make icons themeable (breaking change) (#726)

a18d6ed

* feat(icons): make icons themeable (breaking change)

montezume added a commit that referenced this pull request May 7, 2019

feat(icons): make icons themeable (breaking change) (#726)

2d79410

* feat(icons): make icons themeable (breaking change)

montezume added a commit that referenced this pull request May 7, 2019

feat(icons): make icons themeable (breaking change) (#726)

cdd2dfd

* feat(icons): make icons themeable (breaking change)

montezume added a commit that referenced this pull request May 8, 2019

feat(icons): make icons themeable (breaking change) (#726)

9acf2c4

* feat(icons): make icons themeable (breaking change)

jonnybel pushed a commit that referenced this pull request May 8, 2019

feat(icons): make icons themeable (breaking change) (#726)

e26bd03

* feat(icons): make icons themeable (breaking change)

jonnybel pushed a commit that referenced this pull request May 21, 2019

feat(icons): make icons themeable (breaking change) (#726)

c733823

* feat(icons): make icons themeable (breaking change)

jonnybel pushed a commit that referenced this pull request Jun 4, 2019

feat(icons): make icons themeable (breaking change) (#726)

5c40a3d

* feat(icons): make icons themeable (breaking change)

jonnybel pushed a commit that referenced this pull request Jun 7, 2019

feat(icons): make icons themeable (breaking change) (#726)

ec33306

* feat(icons): make icons themeable (breaking change)

montezume added a commit that referenced this pull request Jun 20, 2019

feat(icons): make icons themeable (breaking change) (#726)

9bea3ad

* feat(icons): make icons themeable (breaking change)

montezume added a commit that referenced this pull request Jun 21, 2019

feat(icons): make icons themeable (breaking change) (#726)

d46cdf6

* feat(icons): make icons themeable (breaking change)

montezume mentioned this pull request Jun 21, 2019

Version 10.0.0 #877

Merged

montezume added a commit that referenced this pull request Jun 24, 2019

feat(icons): make icons themeable (breaking change) (#726)

291092d

* feat(icons): make icons themeable (breaking change)

montezume added a commit that referenced this pull request Jun 25, 2019

feat(icons): make icons themeable (breaking change) (#726)

d5390c3

* feat(icons): make icons themeable (breaking change)

montezume added a commit that referenced this pull request Jul 4, 2019

feat(icons): make icons themeable (breaking change) (#726)

5f1af7d

* feat(icons): make icons themeable (breaking change)

montezume added a commit that referenced this pull request Jul 8, 2019

feat(icons): make icons themeable (breaking change) (#726)

301a9fd

* feat(icons): make icons themeable (breaking change)

montezume added a commit that referenced this pull request Jul 8, 2019

feat(icons): make icons themeable (breaking change) (#726)

d16326e

* feat(icons): make icons themeable (breaking change)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(icons): make icons themeable (breaking change) #726

feat(icons): make icons themeable (breaking change) #726

montezume commented May 2, 2019 •

edited

Loading

emmenko left a comment

emmenko May 2, 2019

emmenko May 2, 2019

tdeekens May 2, 2019

tdeekens May 3, 2019

montezume May 3, 2019

tdeekens left a comment

tdeekens May 2, 2019

montezume commented May 2, 2019

montezume commented May 2, 2019

montezume commented May 3, 2019

montezume May 3, 2019

tdeekens May 3, 2019

aminbenselim left a comment

aminbenselim May 3, 2019

montezume commented May 3, 2019

	+ hintIcon={<WarningIcon color="green" />}
	+ hintIcon={<WarningIcon color="primary" />}

	\| `color` \| `string` \| \| 'solid', 'neutral60', 'info', 'primary40', 'warning', 'error' \| 'solid' \| Specifies the icon color \|
	\| `color` \| `string` \| \| 'solid', 'neutral60', 'info', 'primary', 'primary40', 'warning', 'error' \| 'solid' \| Specifies the icon color \|

feat(icons): make icons themeable (breaking change) #726

feat(icons): make icons themeable (breaking change) #726

Conversation

montezume commented May 2, 2019 • edited Loading

Summary

💣 BREAKING CHANGES 💣

Why do we need themeable icons?

Why are you spelling colour wrong?

emmenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdeekens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

montezume commented May 2, 2019

montezume commented May 2, 2019

montezume commented May 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aminbenselim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

montezume commented May 3, 2019

montezume commented May 2, 2019 •

edited

Loading