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

fix(Icon): Redline matching sizing and spacing of icons in Teams theme #386

Closed
wants to merge 2 commits into from

Conversation

codepretty
Copy link
Collaborator

@codepretty codepretty commented Oct 22, 2018

Update

We've decided to reprocess the Teams SVGs to remove the bounding box. That will change my approach above. Stay tuned as this is on hold until we can process & upload the Teams SVGs


Icons for Teams

For Teams, all icons have a baked in padding. For an icon with a default size the bounding box (viewBox) is 32x32px with a 16x16px icon size.
image

To get an icon font and Teams SVG assets to line up and be able to work together, we need to add in padding for the font icon.

In addition, if there shouldn't be any visible space, the icon needs to be able to "remove" its bounding box. For this, I have updated the Map to include 2 numbers, the bounding box size and the icon size. I can use this to calculate paddings/spacings in the styles.

const sizes = new Map([
...
['normal', [32, 16]],
...
])

By default the icon will show without any space (at least for now since that is what all the other components expect of the icon). However, in order to keep the "pure" SVG, which means to show it with its baked in space (the bounding box), I added all as a new option to the xSpacing prop. So now we can easily line up a row of icons like this
image

With code like this

<div style={{ backgroundColor: 'yellow' }}>
    <Icon name="call-video" xSpacing="all" />
    <Icon name="paperClip" xSpacing="all" />
    <Icon name="book" xSpacing="all" />
    <Icon name="more" xSpacing="all" />
  </div>

@codepretty codepretty added question Further information is requested, concerns that require additional thought are raised RFC labels Oct 22, 2018
@codecov
Copy link

codecov bot commented Oct 22, 2018

Codecov Report

Merging #386 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #386   +/-   ##
=======================================
  Coverage   90.77%   90.77%           
=======================================
  Files          41       41           
  Lines        1334     1334           
  Branches      194      194           
=======================================
  Hits         1211     1211           
  Misses        120      120           
  Partials        3        3
Impacted Files Coverage Δ
src/components/Icon/Icon.tsx 83.33% <ø> (ø) ⬆️

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 3e70307...21347bb. Read the comment docs.

@kuzhelov kuzhelov added the redlines Update of the redlines for the mentioned component label Oct 23, 2018
@kuzhelov
Copy link
Contributor

@codepretty, have tried the changes suggested. In the docs it was initially a bit confusing that for xSpacing we have both and all at the same time, but then I've clarified this moment for myself. In a hope of making the difference more visually appealing in the docs I've tried to adjust styles for this example a bit, and this was what I've ended up with:

image

Is it fine that icon's area would overlap its neighbour elements? Maybe it would be better to adjust padding size instead of margin. Additional and, probably, more appealing reason is that otherwise we might end up with padding areas of Icons being overlapped for the following case:

image

Now, suppose that these icons are clickable, and clickable area is defined by element's size + padding, but not margin. Thus, with this aligning strategy we will end up with very confusing click areas for the icons:

image

image

const IconExampleSpace = () => (
  <div>
    <div style={{ marginBottom: '1.2rem' }}>
      <div style={{ display: 'inline-flex', width: '60px', padding: '10px' border: '1px solid', justifyContent: 'space-around' }}>
        <Icon name="call" styles={{ background: 'grey' }} />
        <Icon name="call-video" styles={{ background: 'yellow' }} />
      </div>
    </div>
  </div>
)

@kuzhelov kuzhelov added needs author feedback Author's opinion is asked and removed question Further information is requested, concerns that require additional thought are raised labels Oct 30, 2018
@codepretty
Copy link
Collaborator Author

@kuzhelov take a look at my new PR and see what you think. I have changed the logic so the icon doesn't have built-in padding anymore, so no more negative margins.

@codepretty codepretty closed this Nov 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs author feedback Author's opinion is asked redlines Update of the redlines for the mentioned component RFC
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants