Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Custom colored toolbar button icons ignore the disabled setting #1035

Open
skateman opened this issue Apr 23, 2018 · 13 comments
Open

Custom colored toolbar button icons ignore the disabled setting #1035

skateman opened this issue Apr 23, 2018 · 13 comments
Assignees

Comments

@skateman
Copy link
Member

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: ManageIQ/ui-components#284

@terezanovotna
Copy link

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

@LHinson
Copy link
Member

LHinson commented Jul 5, 2018

@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
Copy link
Member Author

skateman commented Jul 8, 2018

@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
Copy link

@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
Copy link
Member Author

skateman commented Jul 9, 2018

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

@Loicavenel
Copy link

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

@skateman
Copy link
Member Author

skateman commented Jul 9, 2018

@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
Copy link

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

@Loicavenel
Copy link

@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
Copy link
Member Author

skateman commented Jul 9, 2018

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
Copy link

Seems ok for me..

@andresgalante
Copy link
Member

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
Copy link
Member Author

@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 😞

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants