iTwin / iTwinUI

A design system for building beautiful and well-working web interfaces.
https://itwin.github.io/iTwinUI/
MIT License
104 stars 37 forks source link

UserIcon in IconButton is not properly sized in some scenario. #115

Closed raplemie closed 3 years ago

raplemie commented 3 years ago

Environment

Google Chrome 90.0.4430.212 (Official Build) (64-bit) (cohort: Stable)
Revision e3cd97fc771b893b7fd1879196d1215b622c2bed-refs/branch-heads/4430@{#1429}
OS Windows 10 OS Version 1909 (Build 18363.1556)

Code Sandbox

Could not create a repo as this issue is not occuring in there, not sure how to control that.

Steps to reproduce

<IconButton styleType={"borderless"}>
  <UserIcon
    size={"medium"}
    abbreviation={"IU"}
    backgroundColor={getUserColor("IU")}
  />
</IconButton>

Actual behavior

UserIcon with size "medium" is overriden by default ".iui-button>.iui-icon" in some scenario (which I could not reproduce in code sandbox) when the build output changes the css order...

This is what I get in my application: image

This is what I get in Storybook or Codesandbox examples: image

Expected behavior

I expect the UserIcon to have the specified size even if I put it in a IconButton, so I guess the .iui-user-icon.iui-user-icon should be made even more specific so order does not impact the output.

Additional information

The specificity of the 2 height/width classes are the same (2 classes), so it boils down to the order in which the classes are loaded in the browser. I dont know why my build is putting the .iui-user-icon.iui-user-icon before the .iui-button>.iui-icon, but it does... I'm using a package that uses iTwinui-react components and @bentley/react-scripts, I cant test that in codesandbox as those packages are not publicly available at the moment.

In my test, I was also inside a Header (not set to slim mode), but that didnt seem to add anything regarding the classes. Not that when in slim mode, the usericon actually get to the correct 24px size, instead of the 16px size of iui-icon.

(I must admit that I initially expected the size to be handled by the header in non slim mode too, but it went to the default small size.)

mayank99 commented 3 years ago

Hi Raph. Thanks for the issue.

I dont know why my build is putting the .iui-user-icon.iui-user-icon before the .iui-button>.iui-icon, but it does...

What is the order of css imports at the top of your file? And are you able to reproduce the issue in sandbox if you use the same order?

I must admit that I initially expected the size to be handled by the header in non slim mode too, but it went to the default small size.

This would be an easy override but the issue would still happen outside header.

mayank99 commented 3 years ago

I think the root cause could be fixed in react. (I've created https://github.com/iTwin/iTwinUI-react/pull/95 but it's hacky 😕 )

raplemie commented 3 years ago

Hi Mayank, that's part of the issue, I dont import any CSS in my application, I only use itwinui-react package directly, and through other packages that uses itwinui-react themselves. So there isnt much that I can control or reproduce in the sandbox :(

mayank99 commented 3 years ago

Even if you were able to change the order of imports in your "other package", I think this issue needs a proper fix (we can't always expect users to import everything in the right order, and this is a fairly simple use case).

We could look into increasing the specificity of .iui-user-icon (by repeating the class or by adding a different class or both).

Another solution could be to change the button css like this:

- > .iui-icon {
+ > .iui-icon:not(.iui-user-icon) {
    width: $iui-icons-default;
    height: $iui-icons-default;

@FlyersPh9 Do you know where this could cause problems? (It is increasing specificity)

FlyersPh9 commented 3 years ago

I tested @mayank99 suggestion and it had no negative effect on any of the components. Seems like a good solution.