chore(Indicator): use predefined icon set #1120

mnajdova · 2019-03-27T17:35:02Z

Fixes #896 Added default icons set for the icons used in the Stardust's component:

stardust-close (used in Input and Dropdown)
stardust-arrow-down (used in AccordionTitle and Dropdown)
stardust-arrow-up (used in Dropdown)
stardust-arrow-end (used in AccordionTitle)

It's considered theme's responsibility to implement the rtl behavior of the icons. These icons are added in the base theme as well, by using font awesome icons.

TODO

update changelog

BREAKING CHANGES:

Replace Indicator component with Icon and replace the direction prop with name in the following manner:

end => stardust-arrow-end
bottom => stardust-arrow-down
top => stardust-arrow-up

kuzhelov · 2019-03-28T13:42:10Z

packages/react/src/themes/teams/withProcessedIcons.ts

@@ -12,21 +12,33 @@ const declareSvg = (svgIcon: SvgIconSpec, exportedAs?: string): ThemeProcessedIc
  exportedAs,
 })

-const processedIcons: ThemeIcons = Object.keys(svgIconsAndStyles as {
+const getIcon = iconAndMaybeStyles => {


given that it was decided to extract this method, it would be beneficial if we will specify types for the args and return value (actually, the return value type may be the most beneficial piece)

kuzhelov · 2019-03-28T13:44:32Z

packages/react/src/themes/teams/withProcessedIcons.ts

+    : (iconAndMaybeStyles as SvgIconSpec)
+}
+
+const processedIcons: { [key: string]: ThemeIconSpec } = Object.keys(svgIconsAndStyles as {


you may use ObjectOf<ThemeIconSpec>

Will use ThemeIcons here as well

kuzhelov · 2019-03-28T14:13:20Z

packages/react/src/themes/teams/withProcessedIcons.ts

  [iconName: string]: TeamsProcessedSvgIconSpec
-}).reduce<ThemeIcons>((accIcons, iconName) => {
+}).reduce<{ [key: string]: ThemeIconSpec }>((accIcons, iconName) => {


we can use ThemeIcons at both places where we have { [key: string]: ThemeIconSpec } now - there is no need to use dictionary type. I would suggest to do one of the following:

use ThemeIcons type here and for processedIcons above

or remove ThemeIcons and use ObjectOf<ThemeIconSpec> at both places, if you think that this would be more readable

What do you think?

Yep totally agree, at the first implementation the new names were required that is why I was changing these typings, but yes as they are optional now, I can use the same type. Good catch!

packages/react/src/themes/teams/index.ts

packages/react/src/themes/types.ts

packages/react/src/themes/base/components/Icon/fontAwesomeIconStyles.ts

kuzhelov · 2019-03-28T14:18:31Z

packages/react/src/themes/teams/components/Icon/iconStyles.ts

@@ -130,6 +130,10 @@ const iconStyles: ComponentSlotStylesInput<IconProps, IconVariables> = {
      ...(!rtl && {
        transform: `rotate(${rotate}deg)`,
      }),
+      // override base theme default rtl behavior
+      ...(rtl && {


intent of this and the previous object expansion expressions could be provided in a simpler way:

transfrom: rtl ? 'unset' : `rotate(${rotate}deg)`

kuzhelov · 2019-03-28T14:25:30Z

packages/react/src/themes/base/components/Icon/iconStyles.ts

+        getBorderedStyles(v.borderColor || getIconColor(color, v))),
+
+      ...(rtl && {
+        transform: `scaleX(-1) rotate(${-1 * rotate}deg)`,


just to be sure - seems like rotate prop's value won't have any effect in case if rtl mode is disabled, will it? miss the following style lines from the original file:

...(!rtl && { transform: `rotate(${rotate}deg)`, }),

yep, will add it

-fixed input test

codecov · 2019-03-28T16:52:37Z

Codecov Report

Merging #1120 into master will decrease coverage by 0.06%.
The diff coverage is 54.28%.

@@            Coverage Diff             @@
##           master    #1120      +/-   ##
==========================================
- Coverage   82.09%   82.02%   -0.07%     
==========================================
  Files         724      727       +3     
  Lines        8661     8670       +9     
  Branches     1169     1166       -3     
==========================================
+ Hits         7110     7112       +2     
- Misses       1534     1542       +8     
+ Partials       17       16       -1

Impacted Files	Coverage Δ
packages/react/src/index.ts	`100% <ø> (ø)`	⬆️
...con/svg/ProcessedIcons/icons-triangle-up-small.tsx	`100% <ø> (ø)`	⬆️
packages/react/src/components/Menu/MenuItem.tsx	`61.26% <0%> (-0.35%)`	⬇️
packages/react/src/components/Input/Input.tsx	`100% <100%> (ø)`	⬆️
...mes/teams/components/Icon/svg/icons/triangleUp.tsx	`100% <100%> (ø)`
packages/react/src/themes/base/index.ts	`100% <100%> (ø)`	⬆️
...ckages/react/src/themes/base/componentVariables.ts	`100% <100%> (ø)`	⬆️
packages/react/src/themes/base/componentStyles.ts	`100% <100%> (ø)`	⬆️
...ges/react/src/themes/base/components/Icon/index.ts	`100% <100%> (ø)`
...t/src/themes/base/components/Icon/iconVariables.ts	`100% <100%> (ø)`
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b50927b...d92dd94. Read the comment docs.

kuzhelov · 2019-03-29T09:09:06Z

packages/react/src/components/Menu/Menu.tsx

@@ -225,5 +222,9 @@ class Menu extends AutoControlledComponent<ReactProps<MenuProps>, MenuState> {
 }

 Menu.create = createShorthandFactory({ Component: Menu, mappedArrayProp: 'items' })
+Menu.slotClassNames = {


lets follow the same approach that is introduced for other components (i.e. use the dedicated static field for now) - given that the problem with cyclic dependencies was solved in other way. We might want to reconsider the way slotClassNames are introduced later

kuzhelov · 2019-03-29T09:12:20Z

packages/react/src/themes/teams/components/Icon/fontAwesomeIconStyles.ts

@@ -11,7 +11,7 @@ const brand = (content: string) => fontIcon(content, '"Font Awesome 5 Brands"')
 // Originally generated from:
 // https://github.com/Semantic-Org/Semantic-UI-CSS/blob/master/components/icon.css
 // Corrections were made to duplicate icon names and incorrectly mapped alternates
-const fontAwesomeIcons: ThemeIcons = {
+const fontAwesomeIcons: { [key: string]: ThemeIconSpec } = {


may we use ThemeIcons type here as well?

Yep, changed

layershifter · 2019-03-29T09:16:37Z

packages/react/src/themes/base/components/Icon/iconStyles.ts

+  const modifiedSizes = {
+    large: {
+      x: 24,
+      xx: 28,


I'm not sure that this is part of base theme...

Moved that back to the teams theme. The base theme now is always creating icon with fontSize of 16px

layershifter · 2019-03-29T09:17:18Z

packages/react/src/themes/base/components/Icon/iconStyles.ts

+
+const iconStyles: ComponentSlotStylesInput<IconProps, IconVariables> = {
+  root: ({
+    props: { name, size, bordered, circular, xSpacing, rotate },


As we use p. instead?

bmdalex · 2019-03-29T11:14:09Z

packages/react/src/components/Menu/Menu.tsx

@@ -225,5 +222,9 @@ class Menu extends AutoControlledComponent<ReactProps<MenuProps>, MenuState> {
 }

 Menu.create = createShorthandFactory({ Component: Menu, mappedArrayProp: 'items' })
+Menu.slotClassNames = {
+  divider: `${Menu.className}__divider`,
+  item: `${Menu.className}__item`,


fyi @layershifter @sophieH29 this will fix your issue

bmdalex

changelog and merge 👍

# Conflicts: # CHANGELOG.md # packages/react/src/components/Indicator/Indicator.tsx

-initial commit

713e4a4

mnajdova requested review from dvdzkwsk, kuzhelov, layershifter, levithomason and miroslavstastny as code owners March 27, 2019 17:35

manajdov added 5 commits March 27, 2019 18:51

-temporary adding option icon names

3a3b5e5

-renamed icons

c80f392

Merge branch 'master' into feat/predefined-icon-set

32f64fc

-added base theme icons

b0058c4

-unset rtl styles from base theme icon styles

b9db038

mnajdova changed the title ~~WIP chore(Indicator): use predefined icon set~~ chore(Indicator): use predefined icon set Mar 28, 2019

mnajdova added the 🚀 ready for review label Mar 28, 2019

mnajdova mentioned this pull request Mar 28, 2019

feat(Indicator): add close indicator #1114

Closed

kuzhelov reviewed Mar 28, 2019

View reviewed changes

packages/react/src/themes/teams/index.ts Outdated Show resolved Hide resolved

layershifter reviewed Mar 28, 2019

View reviewed changes

packages/react/src/themes/types.ts Show resolved Hide resolved

layershifter reviewed Mar 28, 2019

View reviewed changes

packages/react/src/themes/base/components/Icon/fontAwesomeIconStyles.ts Outdated Show resolved Hide resolved

kuzhelov reviewed Mar 28, 2019

View reviewed changes

manajdov added 8 commits March 28, 2019 16:28

-addressing comments on the PR

404bc00

-addressing comments on the PR

0343d12

-changed icon names in the MenuItem component

6833c94

-removed color related things from the base theme icon styles

3a84450

-removed unused props

ae806ba

Merge branch 'master' into feat/predefined-icon-set

c3474af

-fixed menu slot class names

5e5f912

-fixed input test

-fixed menu slot class names

795ec30

-fixed input test

-removed Indicator from App.tsx

47048bc

kuzhelov reviewed Mar 29, 2019

View reviewed changes

kuzhelov approved these changes Mar 29, 2019

View reviewed changes

layershifter reviewed Mar 29, 2019

View reviewed changes

manajdov added 3 commits March 29, 2019 11:33

-addressed PR comments

43cd20d

-fixed rotate behavior

74e933e

-reverted recent changes to Menu

47f47f8

bmdalex reviewed Mar 29, 2019

View reviewed changes

manajdov added 2 commits March 29, 2019 12:30

Merge branch 'master' into feat/predefined-icon-set

e05f207

-updated changelog

a9635a6

bmdalex approved these changes Mar 29, 2019

View reviewed changes

manajdov added 2 commits March 29, 2019 13:57

Merge branch 'master' into feat/predefined-icon-set

e85a5f7

# Conflicts: # CHANGELOG.md # packages/react/src/components/Indicator/Indicator.tsx

-reverted changes in the Menu

d92dd94

mnajdova merged commit adae2df into master Mar 29, 2019

delete-merged-branch bot deleted the feat/predefined-icon-set branch March 29, 2019 13:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(Indicator): use predefined icon set #1120

chore(Indicator): use predefined icon set #1120

mnajdova commented Mar 27, 2019 •

edited

Loading

kuzhelov Mar 28, 2019

kuzhelov Mar 28, 2019

mnajdova Mar 28, 2019

kuzhelov Mar 28, 2019

mnajdova Mar 28, 2019

kuzhelov Mar 28, 2019

kuzhelov Mar 28, 2019 •

edited

Loading

mnajdova Mar 28, 2019

codecov bot commented Mar 28, 2019 •

edited

Loading

kuzhelov Mar 29, 2019 •

edited

Loading

kuzhelov Mar 29, 2019

mnajdova Mar 29, 2019

layershifter Mar 29, 2019

mnajdova Mar 29, 2019

layershifter Mar 29, 2019

bmdalex Mar 29, 2019

bmdalex left a comment

chore(Indicator): use predefined icon set #1120

chore(Indicator): use predefined icon set #1120

Conversation

mnajdova commented Mar 27, 2019 • edited Loading

TODO

BREAKING CHANGES:

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

kuzhelov Mar 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 28, 2019 • edited Loading

Codecov Report

kuzhelov Mar 29, 2019 • edited Loading

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

Choose a reason for hiding this comment

bmdalex left a comment

Choose a reason for hiding this comment

mnajdova commented Mar 27, 2019 •

edited

Loading

kuzhelov Mar 28, 2019 •

edited

Loading

codecov bot commented Mar 28, 2019 •

edited

Loading

kuzhelov Mar 29, 2019 •

edited

Loading