search

patternfly / patternfly-3

This repo contains the HTML, CSS, and JQuery for the PatternFly 3 project.
https://www.patternfly.org/v3
MIT License
1.15k stars 239 forks source link

Custom colored toolbar button icons ignore the disabled setting #1035

Open skateman opened 6 years ago

skateman commented 6 years ago

We need to support custom colored button icons in ManageiQ, but it seems like if I have to set the opacity on the color manually if I want to have it look consistently disabled with the button text.

It looks like:

But it should look like:

Is there any way to adjust the disabled class in patternfly to apply on the custom colored fonticons?

Related BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1501114 Workaround: https://github.com/ManageIQ/ui-components/pull/284

terezanovotna commented 6 years ago

@LHinson any idea who's a good person to ping about this?

LHinson commented 6 years ago

@skateman @terezanovotna To confirm, the icons do not look disabled because of this customization? If you used the standard color, would the icon look disabled?

From a user experience perspective, I don't see any reason why edit, policy or download (as seen in the screenshot above) require a color. What value does that provide? The goal of PatternFly is to provide consistency across the various UIs, therefore I'd recommend removing this customization and opt for the PF recommended colors to align with the broader set of products.

@terezanovotna @skateman

skateman commented 6 years ago

@LHinson the problem here is inconsistency, if the button has no custom color (i.e. has the default one) and it becomes disabled, its color will be adjusted: screenshot from 2018-07-08 08-14-53

terezanovotna commented 6 years ago

@LHinson has a great point about color. It's nice to display color, but it doesn't provide any value to the user, and therefore having the icons black (for active) and grey (for disabled) makes sense.

@skateman would that resolve the issue?

skateman commented 6 years ago

@terezanovotna @LHinson this was a requested feature by @Loicavenel

Loicavenel commented 6 years ago

@skateman I cannot remember this, I know this was requested for Custom Button

skateman commented 6 years ago

@Loicavenel yes, this is the custom buttons feature with custom colors. As far as I understand @terezanovotna is suggesting to drop the coloring and have all icons gray, so we don't have to deal with the opacity when disabled.

terezanovotna commented 6 years ago

@skateman is right. Would it be a reasonable solution? @Loicavenel

Loicavenel commented 6 years ago

@terezanovotna we introduced this feature in CF 4.6 and people really like the capability to set color on the 811 icons option, not sure we should removed a feature already supported

skateman commented 6 years ago

So the thing I am proposing here is to adjust the definition of the .diabled class on a button's icon. Right now PF explicitly sets a color with the evil !important. Instead of this the opacity should be set to let's say 50% on the color that is being set on the non-disabled button. This way it is not important what color is being set for the icon, it will have the disabled look & feel.

Loicavenel commented 6 years ago

Seems ok for me..

andresgalante commented 6 years ago

Hi @skateman you are right, it should be an opacity. I have 2 concerns:

  1. if we make it a transparency of an unknown color, let's make sure the percentage we pick is readable and accessible.
  2. Youy are riught, there shouldn't be an important there and looking in the git history I can't spot how did it end up there. Having said we should make sure that we are not breaking anything by removing it.

@skateman will you be able to send a PR to fix this?

skateman commented 6 years ago

@andresgalante I was looking into this, but couldn't come up with a fix, that's why I created the issue and not a PR :disappointed: