Override fluentui-blazor error hex, darken trace duration background #1240

adamint · 2023-12-06T15:14:15Z

For #689, I darkened the background color, which looks like this on light and dark theme:

Both contrasts meet WCAG guidelines for text on background, and between the row and package backgrounds. Other color suggestions are welcome as well. This is ##938989

For #690, I override --error to be more red, as this is an issue in the three places that Color.Error shows up. We could ask to change the default in fluentui-blazor. This is one of the lightest possible shades we can choose, with a dark theme contrast of ~3.0.

tlmii · 2023-12-06T22:36:45Z

For #689, the subtext still has contrast issues with the new duration color in both light and dark mode:

One suggestion we had gotten from UX folks was to make the subtext color the same as the regular color when it is on a highlighted background like that (and that's what I did in #1231 ). But in this case we have no way to know whether the subtext is over the duration color or the regular background color.

adamint · 2023-12-06T22:38:08Z

I didn't consider the subtext. We could make it the same color if any of the row is highlighted?

tlmii · 2023-12-06T22:47:39Z

src/Aspire.Dashboard/wwwroot/css/app.css

@@ -151,6 +151,10 @@ fluent-data-grid-cell .long-inner-content {
    color: var(--neutral-foreground-rest);
 }

+ :root {
+     --error: #F8040A !important; /* the default error color (#BC2F32) is not accessible in dark theme */


@vnbaaij I think the !important here is necessary because variables is included dynamically via the blazor module injection (indirectly via the Microsoft.FluentUI.AspNetCore.Components.css) so it comes last and we can't override with our own. Is there any way to avoid that, especially since there's so little in that file? I guess this will all change in 4.2 because of Javier's changes, right? So maybe best to leave this and deal with it after that?

What changes are occurring in 4.2?

This is what I was referring to: microsoft/fluentui-blazor#1051
That moved some of the CSS into JS and adopted stylesheets. E.g. what was in variables.css is now here: https://github.com/microsoft/fluentui-blazor/blob/34b60e1bcd924dbef9ce3e24ae3de76ae3f21711/src/Core.Assets/src/index.ts#L15C1-L29C2

Asked for clarity on when that will be released.

tlmii · 2023-12-06T22:49:27Z

I didn't consider the subtext. We could make it the same color if any of the row is highlighted?

The problem here is that its not really highlighted vs not - every row will have some duration, so there'll always be some of this duration background (it may be so small you can't see it, but where it falls compared to the rest of the contents of the row is based on the duration and the screen size, etc).

adamint · 2023-12-07T17:55:22Z

every row will have some duration

But in this case we have no way to know whether the subtext is over the duration color or the regular background color

It seems to me like the right solution is just to make the subtext the same as the main text color, as it has sufficient contrast on both backgrounds

tlmii · 2023-12-07T21:47:16Z

If we remove the ghosting of the text, should we do something else to set it off from the rest of the text there? Put it in paren? Separate it with a pipe? Or is it ok as is? @JamesNK guessing you'll have thoughts on that.

JamesNK · 2023-12-08T01:09:59Z

Feedback:

The new colors are too light and too dark. It looks ugly. The colors used for row selection when a details pane is open would be better.
I'm guessing the ghosted text is the problem. For now, make it the same color as the other text. It is smaller and there is some padding to differentiate it from the rest of the title. If it is confusing, then we'll try to improve it more in the future.

JamesNK · 2023-12-08T01:11:48Z

src/Aspire.Dashboard/wwwroot/css/app.css

@@ -151,6 +151,10 @@ fluent-data-grid-cell .long-inner-content {
    color: var(--neutral-foreground-rest);
 }

+ :root {
+     --error: #F8040A !important; /* the default error color (#BC2F32) is not accessible in dark theme */


There is a :root section earlier in this file. Move this variable to it.

adamint · 2023-12-08T17:06:00Z

The new colors are too light and too dark

The set values are at the edge of allowable contrast for light theme - it can be darkened slightly in dark theme. At least for gray, it is not possible to go lighter (for light theme). @JamesNK

The ghosted text is a problem, but is not the only problem.

adamint · 2023-12-11T16:53:07Z

waiting on this until 4.2.0 of fluent components so the !important can be removed

JamesNK · 2023-12-11T23:17:16Z

The set values are at the edge of allowable contrast for light theme - it can be darkened slightly in dark theme. At least for gray, it is not possible to go lighter (for light theme). @JamesNK

I'm confused about this statement because other colors throughout the app already have lighter backgrounds:

Here is dark mode:

The colors used to highlight the details pane look much better.

adamint · 2023-12-12T15:28:06Z

I'm confused about this statement because other colors throughout the app already have lighter backgrounds:

@JamesNK Those are different scenarios, where there is no required contrast ratio. Here, being that the color indicates state, we are bound by WCAG 1.4.11 (see link for specific criteria) to ensure a contrast ratio of 3:1.

The colors used to highlight the details pane look much better.

Aesthetically I agree with you, but that isn't relevant to the contrast problem. I'm happy to suggest a range of different colors for your thoughts, or we can indicate the trace's duration as a % of total in some other way.

adamint · 2023-12-13T17:08:01Z

@tlmii and @drewnoakes as well for thoughts ^

JamesNK · 2023-12-14T03:42:53Z

I'd rather remove the trace duration indicator that have it look like this.

adamint · 2023-12-14T03:45:20Z

We could also indicate percentage using a progress bar or ring. What do you think of that idea @JamesNK

JamesNK · 2023-12-14T03:50:11Z

I'm not sure what you mean. Each row is kind of a progress bar today.

Are you thinking of adding a very small graph in the grid? The problem with adding new content is screen real-estate is limited, so something else will have to be made smaller.

adamint · 2023-12-14T03:52:04Z

Not a graph, but adding a fluent Progress https://www.fluentui-blazor.net/Progress or possibly progress circle component somewhere in the row

tlmii · 2023-12-14T04:48:39Z

We run into the problem that unless it is very large (like it is now, where it takes up the whole row), it can be hard to tell relative lengths (other than the overall vs all the smaller ones), so I'd worry making it only take up one column's width or something would diminish the value too much.

I do think this is worth getting an a11y expert opinion on this at some point. @kvenkatrajan or @leslierichardson95 Do you think we should take this to office hours? I don't think we really discussed this particular one during the UX board meeting a couple weeks ago. I think there's value in the subtle duration bars, if we can come up with a way to ensure the same information is available to everyone (note that I don't think it has to be making the subtle duration bars meet all the rules if we can have an additional way to display the same info).

JamesNK · 2023-12-14T05:33:03Z

I played around with a percentage ring icon. I think it's not bad. PR here: #1387

adamint · 2023-12-14T05:41:10Z

(note that I don't think it has to be making the subtle duration bars meet all the rules if we can have an additional way to display the same info).

Right, the complication is ensuring it is available in some way to non-visual users. I would suggest a progress ring if we do want to convey this information.

JamesNK · 2023-12-15T07:07:09Z

#1387 is merged. This PR can be simplified to changing the error color.

adamint · 2023-12-18T21:42:35Z

Simplifying PR to just changing error color & updating fluentui. Still using this PR to keep the conversation history

adamint · 2023-12-18T22:03:41Z

Just modified for dark theme, as light theme --error can stay the same. !important is still needed, unfortunately. @tlmii @drewnoakes

adamint requested review from JamesNK, drewnoakes and tlmii December 6, 2023 15:14

ghost added the area-dashboard label Dec 6, 2023

adamint force-pushed the dev/adamint/contrast-traces branch from 7951ed4 to 656e4b2 Compare December 6, 2023 15:20

tlmii reviewed Dec 6, 2023

View reviewed changes

JamesNK reviewed Dec 8, 2023

View reviewed changes

adamint closed this Dec 18, 2023

adamint reopened this Dec 18, 2023

adamint closed this Dec 18, 2023

adamint force-pushed the dev/adamint/contrast-traces branch from f47dd78 to 4e334a5 Compare December 18, 2023 21:40

Adam Ratzman added 3 commits December 18, 2023 16:43

Update fluentui-blazor

e70337f

import fluentui-blazor from the right path

679ac1d

lighten --error in dark theme

4359b17

adamint reopened this Dec 18, 2023

add !important to --error override

ad9fafb

adamint requested review from tlmii and JamesNK December 18, 2023 22:05

JamesNK approved these changes Dec 18, 2023

View reviewed changes

adamint merged commit 7df8909 into dotnet:main Dec 18, 2023

github-actions bot locked and limited conversation to collaborators Apr 27, 2024

Override fluentui-blazor error hex, darken trace duration background #1240

Override fluentui-blazor error hex, darken trace duration background #1240

Uh oh!

Conversation

adamint commented Dec 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tlmii commented Dec 6, 2023

Uh oh!

adamint commented Dec 6, 2023

Uh oh!

tlmii Dec 6, 2023

Choose a reason for hiding this comment

Uh oh!

adamint Dec 6, 2023

Choose a reason for hiding this comment

Uh oh!

tlmii Dec 6, 2023

Choose a reason for hiding this comment

Uh oh!

adamint Dec 7, 2023

Choose a reason for hiding this comment

Uh oh!

tlmii commented Dec 6, 2023

Uh oh!

adamint commented Dec 7, 2023

Uh oh!

tlmii commented Dec 7, 2023

Uh oh!

JamesNK commented Dec 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JamesNK Dec 8, 2023

Choose a reason for hiding this comment

Uh oh!

adamint commented Dec 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamint commented Dec 11, 2023

Uh oh!

JamesNK commented Dec 11, 2023

Uh oh!

adamint commented Dec 12, 2023

Uh oh!

adamint commented Dec 13, 2023

Uh oh!

JamesNK commented Dec 14, 2023

Uh oh!

adamint commented Dec 14, 2023

Uh oh!

JamesNK commented Dec 14, 2023

Uh oh!

adamint commented Dec 14, 2023

Uh oh!

tlmii commented Dec 14, 2023

Uh oh!

JamesNK commented Dec 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamint commented Dec 14, 2023

Uh oh!

JamesNK commented Dec 15, 2023

Uh oh!

adamint commented Dec 18, 2023

Uh oh!

adamint commented Dec 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

adamint commented Dec 6, 2023 •

edited

Loading

JamesNK commented Dec 8, 2023 •

edited

Loading

adamint commented Dec 8, 2023 •

edited

Loading

JamesNK commented Dec 14, 2023 •

edited

Loading

adamint commented Dec 18, 2023 •

edited

Loading