Badge : Add some preset theme color and make backgroundColor props optional

maxime4000 commented 3 years ago

Feature

I want to use Badge, I want to use some color when light theme and other color when dark theme. Right now, there is "no way" to do that. If I set a classname, I'm required to leave a backgroundColor and !important my way to override it or I need to code the color props send to the component depending on the theme.

My Request :

Add default theme to badge.
- Something like : Positive, Negative, Warning, Black, White, and the data-viz color as preset option.
Add new TextColor and Border props as optional, make backgroundColor props as optional too.
- It's bad practice to !important CSS rule but the required props force that.
If you want to set color, your own classname or style props should do enough.
- Leave CSS props as shortcut, but not required.
Badge should have default color if nothing is set. e.g. <Badge>Label<Badge>

Exemple :

<Badge backgroundColor="#ffffff" border="1px solid black" textColor="black">
  Label
</Badge>
<Badge style={{backgroundColor:"#ffffff", border:"1px solid black", textColor:"black"}}>
  Label
</Badge>
<Badge theme="Positive">
  Label
</Badge>
<Badge className="CustomBadge">
  Label
</Badge>

maxime4000 commented 3 years ago

Maybe add Props for text transform: uppercase ? https://github.com/iTwin/iTwinUI/issues/180

mayank99 commented 3 years ago

Are you aware of the getUserColor function that is designed specifically for applying data-viz colors?

<Badge backgroundColor={getUserColor('Label')}>Label</Badge>

If you don't like those colors, you can pass empty string '' to backgroundColor and then apply your color through css without needing !important.

<Badge backgroundColor='' className='CustomBadge'>Label</Badge>

I do agree that the API (or at least documentation) can be improved a bit, as it's not exactly intuitive currently. We'll see what we can do.

maxime4000 commented 3 years ago

Honestly I don't understand why getUserColor would help. The code is taking a string and choosing a color randomly depending on the hash... It explain why my userIcon is always the "Ash" color and why other user has different color, but it doesn't provide consistency. I think the more appropriate solution for that would be to import an enum :

export enum DataVizColor {
  Skyblue = "#6ab9ec", // UserColor[0]
  Celery = "#b1c854", // UserColor[1]
  ...
}

But my suggestion here is to have a list of built-in theme usable. Same thing that Button has : cta, default, high-visibility and borderless (I guess this apply to iTwinUI-CSS too)

If backgroundColor="" work why not making it optional ? My suggestion here is that component are written by the book with little flexibility. Real app scenario are different from designed scenario and some component can be reuse in other scenario, but because they aren't flexible, the users are needed to work some hack for it to work. Taking UPPERCASE transform as example, default setting should be that text is transform, but if a props called idk textTransform={false} is set, the text is not transform and the user is happy and the dev is following the standard. Everyone is happy.

I'm making issue here to help iTwinUI/React become something reliable and more useful. I already have my fix done in my app. My goal here is that in a near future, I can remove the hack code I place to achieve what I want and rely only on the API to achieve the same.

mayank99 commented 3 years ago

If backgroundColor="" work why not making it optional ?

But my suggestion here is to have a list of built-in theme usable. Same thing that Button has

We don't normally want users to use it without a backgroundColor. Or at least we want to make it hard.

The way our Badge standard is designed, it is not supposed to have a "default" color. Maybe we can change that. @FlyersPh9

Our initial thought was that you should just import those data-viz color variables directly in your scss file, and use it from there. But it directly contradicts the backgroundColor point above, so we'll consider reworking something here.

component can be reuse in other scenario, but because they aren't flexible, the users are needed to work some hack for it to work. Taking UPPERCASE transform as example, default setting should be that text is transform, but if a props called idk textTransform={false} is set, the text is not transform

Outside the backgroundColor scenario, all current behavior is perfectly valid, and I don't see css overrides as a "hack". The uppercase transform is how all badges are supposed to always look, and we'd rather not have prop clutter for custom use cases, especially since we expose styling props specifically for that purpose.

If there were additional problems like css specificity stopping you from doing css overrides, then it would be a different story. But for Badge, that is not the case.

mayank99 commented 3 years ago

make backgroundColor props as optional

Badge should have default color if nothing is set.

Something like : Positive, Negative, Warning

All of this is now available in iTwinUI-react 1.15.0 🚀

Additionally:

Badge styling was completely revamped for increased accessibility.
the backgrounColor prop now supports intellisense for preset colors.

iTwin / iTwinUI-react