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

Change traces page duration progress bar to a ring icon #1387

Merged
merged 5 commits into from
Dec 15, 2023

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented Dec 14, 2023

Fixes #689

a11y requirements make the trace progress bar problematic. A potential replacement is to replace the progress bar with an icon.

Looks pretty good to me. Hopefully no a11y issues? 🙏 Has a benefit of being in the duration column so it's obvious what it's for. Some people either didn't notice the progress bar or didn't know what it meant.

Screenshot:
image

Thoughts?

The PR code is hacky. If we like this then I'll clean it up.

Microsoft Reviewers: Open in CodeFlow

@adamint
Copy link
Member

adamint commented Dec 14, 2023

Why is the FluentProgressRing component not being used? I would strongly suggest using it.

Overall, I do like this idea a lot

fyi: you can do a quick accessibility pass using Accessibility Insights for web

@JamesNK
Copy link
Member Author

JamesNK commented Dec 14, 2023

Why is the FluentProgressRing component not being used? I would strongly suggest using it.

FluentProgressRing doesn't have a way to change the stroke width, i.e. the thickness of the circle line. It is too thin when it is a small size and doesn't look good.

And FluentProgressRing doesn't support completing the remainder of the circle with another color. Without it, many rows have a small dot of purple which doesn't make sense. There is no indication it is part of a greater circle.

image

@adamint
Copy link
Member

adamint commented Dec 14, 2023

@vnbaaij are these improvements possible? I see these properties are configurable in other fluent ui wrappers

Copy link
Member

@adamint adamint left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you double check that this passes Accessibility Insights check? I wonder if the svg needs labeling

@tlmii
Copy link
Member

tlmii commented Dec 14, 2023

We can style most of the FluentProgressRing ourselves as is. This is with the FluentProgressRing and just giving a color to the rest of the ring. I didn't change the stroke width. I think it looks ok. This is with adding:

::deep fluent-progress-ring::part(background) {
    stroke: var(--neutral-fill-input-alt-active);
}

to the Traces.razor.css file on your branch, James.

image
image

@JamesNK
Copy link
Member Author

JamesNK commented Dec 14, 2023

Good improvement. I'll stick to my guns on it looking better with a thicker line.

How about using custom SVG for now, and if Fluent UI adds an option in the future for changing the stroke width, then we switch to FluentProgressRing?

@JamesNK
Copy link
Member Author

JamesNK commented Dec 14, 2023

Created a feature request for the FluentUI folks: microsoft/fluentui-blazor#1113

@tlmii
Copy link
Member

tlmii commented Dec 14, 2023

We can get close to what you've got with just CSS. It's a little chunkier than yours because we can't change the viewbox, but this is 4px stroke width like yours:

image
image

::deep fluent-progress-ring::part(background) {
    stroke: var(--neutral-fill-input-alt-active);
    stroke-width: 4px;
    r: 6px;
}

::deep fluent-progress-ring::part(determinate) {
    stroke-width: 4px;
    r: 6px;
}

We do get a bunch of aria attributes with the FluentProgressRing that'll be helpful for accessibility... just need to add a title attribute and it'll pass, I think. But I'm fine either way. Just trying to provide options.

@JamesNK
Copy link
Member Author

JamesNK commented Dec 14, 2023

Updated to use progress ring control and passed a11y tool check.

@dvoituron
Copy link
Contributor

We can get close to what you've got with just CSS. It's a little chunkier than yours because we can't change the viewbox, but this is 4px stroke width like yours:

This CSS seems correct, but the Value no longer corresponds to the result. For example, 50% no longer displays progress at 50% 😀 You need to add stroke-dashoffset.

@JamesNK
Copy link
Member Author

JamesNK commented Dec 14, 2023

Thanks, I see it's definitely off when I hardcode 25%, 50% and 75% values.

I don't think there is a constant stroke-dashoffset that works for all values. I tried some values out and the ring is either too big or too small at 25% and 75%. Lower-level support is needed in the control.

@dvoituron
Copy link
Contributor

In deed, I'm looking into the possibility of adding a FluentprogressRing.Stroke property, which would be an enumeration of the "normal", " large", " thin" type. This would simplify management of this value.

@dvoituron
Copy link
Contributor

@kvenkatrajan kvenkatrajan added this to the preview 3 (Jan) milestone Dec 14, 2023
@dvoituron
Copy link
Contributor

Will be included in the next release of FluentUI.Blazor lib.
See PR microsoft/fluentui-blazor#1121

@JamesNK JamesNK enabled auto-merge (squash) December 15, 2023 02:12
@JamesNK JamesNK merged commit 6ba39d8 into main Dec 15, 2023
8 checks passed
@JamesNK JamesNK deleted the jamesnk/trace-progress-icon branch December 15, 2023 02:33
@github-actions github-actions bot locked and limited conversation to collaborators Apr 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Contrast issues in duration bar on /Traces
5 participants