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

fix(avatar): sizing issues #976

Merged
merged 16 commits into from
Feb 27, 2019
Merged

fix(avatar): sizing issues #976

merged 16 commits into from
Feb 27, 2019

Conversation

kuzhelov
Copy link
Contributor

@kuzhelov kuzhelov commented Feb 26, 2019

!! Important follow-up

Please, note that box-sizing issue is, essentially, the one that we might expect for other components to appear. This becomes to be an issue after static styles of Semantic UI were eliminated - one of the rules they had introduced was box-sizing one.

TODO

  • update changelog

Appearance of Avatar component in Teams High Contrast theme was fixed. Specifically, the following
aspects were addressed:

Avatar sizing issue

When theme switch to Teams Hight Contrast (and, apart from other style changes, border being introduced to Avatar), Avatar's size was increased by border's thickness value:

Teams default theme (size is 32px)

image

Teams High Contrast theme (size has changed to 32px + 2 * 2px)

image

This has happened because of absence of box-sizing: border-box CSS rule.

Font-based Icon component

When is provided into image slot of Avatar component (originally reported here #795):

Was

image

Now

image

image

@codecov
Copy link

codecov bot commented Feb 26, 2019

Codecov Report

Merging #976 into master will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #976      +/-   ##
========================================
- Coverage   81.01%    81%   -0.01%     
========================================
  Files         665    665              
  Lines        8527   8528       +1     
  Branches     1442   1443       +1     
========================================
  Hits         6908   6908              
- Misses       1605   1606       +1     
  Partials       14     14
Impacted Files Coverage Δ
...t/src/themes/teams/components/Image/imageStyles.ts 25% <ø> (ø) ⬆️
...src/themes/teams/components/Avatar/avatarStyles.ts 33.33% <ø> (ø) ⬆️
...act/src/themes/teams/components/Icon/iconStyles.ts 22.58% <0%> (-0.38%) ⬇️

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 b6afb18...e46e359. Read the comment docs.

@@ -26,6 +27,7 @@ const avatarStyles: ComponentSlotStylesInput<AvatarProps, any> = {
}
},
image: ({ variables: v }): ICSSInJSStyle => ({
boxSizing: 'border-box',
Copy link
Contributor

Choose a reason for hiding this comment

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

I saw that you added the same thing in the image, that would mean that we don't need i there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, but this is, actually, necessary, as not only the Image component could be rendered here. Essentially, we would like to ensure that all sizing props defined for this slot will be applied in the expected way

@@ -31,6 +31,7 @@ const radioStyles: ComponentSlotStylesInput<

icon: ({ props: p, variables: v }): ICSSInJSStyle => ({
margin: `0 ${pxToRem(10)} 0 0`,
padding: 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

I am confused with this change... Can you explain what this was added?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, have addressed related visual regression in icon's styles, thank you!

@kuzhelov kuzhelov merged commit 7ab5611 into master Feb 27, 2019
@delete-merged-branch delete-merged-branch bot deleted the fix/avatar-icon-size branch February 27, 2019 12:19
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.

None yet

3 participants