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

feat(Icons): Adding Office icons #938

Merged
merged 11 commits into from
Mar 8, 2019
Merged

feat(Icons): Adding Office icons #938

merged 11 commits into from
Mar 8, 2019

Conversation

codepretty
Copy link
Collaborator

@codepretty codepretty commented Feb 20, 2019

Adding the Office icons. In a future task, the idea is to have each icon be exported, so we can specify what props are supported (eg outline, color, sizes).

When that work is completed, we will need to update the [office name]-color icons to indicate they are only supported at 32px size and keep their brand colors. The [office name] icons will need to specify they don't support outline prop too.

New icons: Word, Excel, Powerpoint, Onenote

image

How to use

<Icon name="word" />
image

<Icon name="word-color" size="larger" />
image

Usages in the doc examples

Attachment
image

Menu
image

@codepretty codepretty added the question Further information is requested, concerns that require additional thought are raised label Feb 20, 2019
@codecov
Copy link

codecov bot commented Feb 20, 2019

Codecov Report

Merging #938 into master will increase coverage by 0.07%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #938      +/-   ##
==========================================
+ Coverage   81.66%   81.74%   +0.07%     
==========================================
  Files         677      687      +10     
  Lines        8766     8804      +38     
  Branches     1492     1558      +66     
==========================================
+ Hits         7159     7197      +38     
  Misses       1592     1592              
  Partials       15       15
Impacted Files Coverage Δ
...ponents/Icon/svg/ProcessedIcons/icons-msft-ppt.tsx 100% <ø> (ø)
...nts/Icon/svg/ProcessedIcons/icons-msft-onenote.tsx 100% <ø> (ø)
...mponents/Icon/svg/ProcessedIcons/icons-msft-xl.tsx 100% <ø> (ø)
...onents/Icon/svg/ProcessedIcons/icons-msft-word.tsx 100% <ø> (ø) ⬆️
...emes/teams/components/Icon/svg/icons/filesWord.tsx 100% <100%> (ø)
...mes/teams/components/Icon/svg/icons/filesExcel.tsx 100% <100%> (ø)
...teams/components/Icon/svg/icons/filesWordBrand.tsx 100% <100%> (ø)
...ms/components/Icon/svg/icons/filesOneNoteBrand.tsx 100% <100%> (ø)
...s/teams/components/Icon/svg/icons/filesOneNote.tsx 100% <100%> (ø)
...eams/components/Icon/svg/icons/filesExcelBrand.tsx 100% <100%> (ø)
... and 15 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 3609adc...ae95b4c. Read the comment docs.

@rohitpagariya
Copy link

  1. Should we indicate an icon isn't supported for 'outline' or 'filled' version or just let the user notice that when they try to use an icon?

I would prefer if we let the user know if something is not supported. I spent quite some time trying to debug why an icon outline wouldn't render, before realizing it wasn't supported. Generally, I recommend never to fail silently.

@codepretty codepretty added the RFC label Feb 21, 2019
@bcalvery
Copy link
Contributor

bcalvery commented Feb 21, 2019

Should we indicate an icon isn't supported for 'outline' or 'filled' version or just let the user notice that when they try to use an icon?

Are we talking a programattic way to indicate that the icon shouldn't have an outlined version? I think it wouldu be sufficient to just have a comment to that effect in the icon itself.
Then again, maybe the "-color" extension is enough of an indication in it's own right?

Should we indicate when an icon has a different viewBox than the other icons, like in the case of the full color icon? I don't particularly like the name 'filesWordFullColor32', but not sure how else to signal what it is.

What are the implications of the non-standard viewbox?

@@ -8,4 +8,5 @@ export default {
</svg>
),
styles: {},
exportedAs: 'word word-color',
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe it would be better to use comma as separator here - then it would be clear that there are two icons exported for that, and that it is not just a single icon exported under word word-color name

@mnajdova
Copy link
Contributor

mnajdova commented Mar 1, 2019

  1. Should we indicate an icon isn't supported for 'outline' or 'filled' version or just let the user notice that when they try to use an icon?
  2. Should we indicate when an icon has a different viewBox than the other icons, like in the case of the full color icon? I don't particularly like the name 'filesWordFullColor32', but not sure how else to signal what it is.
  1. The best option is that yes, we should indicate somehow. We are converting the outline now to be prop, but the problem is that the icons are theme dependent. I don't have any idea how we can do this at this moment.
  2. Why would some icon have different viewBox than other icons. I thought we have the size prop for this. It would be really strange, if the user tries to use side by side several icons (for example in menu, and some of the icons will be bigger then other, although hey have the same size...)

@codepretty
Copy link
Collaborator Author

  1. Why would some icon have different viewBox than other icons. I thought we have the size prop for this. It would be really strange, if the user tries to use side by side several icons (for example in menu, and some of the icons will be bigger then other, although hey have the same size...)

There are some cases where you won't want to scale the icon, because while you want the icon to be larger or smaller, you wouldn't necessarily want the paths to show differently. In those cases, design has provided icons for the different sizes.

@codepretty
Copy link
Collaborator Author

I would prefer if we let the user know if something is not supported. I spent quite some time trying to debug why an icon outline wouldn't render, before realizing it wasn't supported. Generally, I recommend never to fail silently.

Future plan is to export each icon separately. That way each icon can specify which props it can use, outline/filled/size/etc. There is a separate issue for that work.

@codepretty codepretty added 🚀 ready for review and removed RFC question Further information is requested, concerns that require additional thought are raised labels Mar 8, 2019
@codepretty codepretty merged commit 6ce749c into master Mar 8, 2019
@delete-merged-branch delete-merged-branch bot deleted the feat/add-office-icons branch March 8, 2019 22:27
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.

6 participants