Add label and valueText props to EuiProgress

[andreadelrio]

Summary

This PR adds the label and valueText props to be used with determinate EuiProgress when creating bar charts layouts.

EuiText does not support primary as a color so I created a helper class to be able to apply $euiColorPrimary to the label when color is set to primary for EuiProgress.

I also added the corresponding example to the docs and a snippet to another example.

Fixes #3652

Checklist

• Check against all themes for compatibility in both light and dark modes
• Checked in mobile
• Checked in IE11 and Firefox
• Props have proper autodocs
• Added documentation
  - [ ] Checked Code Sandbox works for the any docs examples
• Added or updated jest tests
  - [ ] Checked for breaking changes and labeled appropriately
  - [ ] Checked for accessibility including keyboard-only and screenreader modes
• A changelog entry exists and is marked appropriately

[kibanamachine]

Preview documentation changes for this PR: https://eui.elastic.co/pr_3661/

[myasonik]

I've got a few a11y concerns about this...

In #2983 I bring up the <progress> vs <meter> discrepancy. This design is expressly a <meter> element, and not a <progress> element. Last time this was brought up, discussion on how to proceed basically stalled out.

This is further complicated by looking at some accessible demos of each:
- https://scottaohara.github.io/a11y_styled_form_controls/src/progress-bar/
- https://scottaohara.github.io/a11y_styled_form_controls/src/meter/

In the progress bar code snippets, we can see that he uses a <label> element while in the meter examples, he doesn't. This is intentional because of the poor support for labelled meters, even though as form elements, both should support this. The demo pages (March 2019 and Nov 2018, respectively) so we should retest his findings to find out if we can simplify this.

Even ignoring the <progress> vs <meter> discrepancy, the "label" for this should render as a <label> for <progress> elements but creating a custom label prop here then runs into difficulties with using EuiProgress inside EuiFormRow which will try to wire up its own <label>. (Putting it into the bucket of "difficult" components which is a growing headache: #3218 and #2493.)

I realize that this is a lot to bring up for a small PR that's trying to unify a pattern that's found in a couple places in Kibana but once it's in EUI, patterns tend to spread and become more difficult to retrofit accessibility onto.

[cchaos]

Thanks for the detailed explanation @myasonik! Do you have advice on how to actually proceed?

runs into difficulties with using EuiProgress inside EuiFormRow

I'm honestly not too worried about this, it doesn't seem like a valid occurrence to begin with. We can always put up a callout in our docs mentioning that EuiProgress should not be contained within an EuiFormRow.

[snide]

Sorry if I'm underthinking the problem here. But the visual of the bar isn't needed for a screenreader if the text content is there. Can't we just aria hide the bar in this case?

I get the meter concern if it was alone on it's own, but that's sort of a separate PR. For here we're just using it as a visual indicator along with text content, so it's easy enough to just hide it.

Agree we should make a separate meter component at a later time though.

[myasonik]

Can't we just aria hide the bar in this case?

@dave You're right, we could hide the <progress /> bar here because it's just a visual indicator of what the text already says in this case... But, at least right now, the text displayed is entirely free-form so we can't do this in the component. If the component prescribed the text, then sure.

Do you have advice on how to actually proceed?

@cchaos The two options I see are:

1. Hide the <progress /> bar with aria and perscribe the valueText in the component (cast the value to a string, and stick a percent on it in an EuiI18n string) though I'm not sure if that actually covers all of Kibana's use cases that we're trying to solve
2. Don't do this in EUI yet, leave it up to consuming apps to roll an accessible pattern for now, and revisit this once the other tickets I linked in the earlier comment are resolved.

[cchaos]

Accessibility concerns aside for the moment, I think this is a great first pass display/design-wise. Now, what we want to do is evaluate it from a flexibility and code maintenance side. Consider these couple things

1. Is using <EuiText> necessary? It comes with a lot of baggage that once we get into a more modular structure, won't be necessary for this particular component. Like the fact that .euiText is a giant blob of selectors and styles. Consider, just using simple spans with classes and using our text mixins.
2. There are now a lot of wrapping divs, can these be simplified?
3. We're continuing to see consumer needs to pass props to inner elements. We should consider alway adding some sort of labelProps prop that allows them to pass anything to these elements.
4. Should valueText be created based on value? Instead of asking them for this value again, what if we just spit out ${value}%? Then we'd change the prop to be some sort of enum or boolean.
5. Thoughts on making the Amsterdam theme display the progress bar with fully rounded corners?

[snide]

For the accessibility side, let's go with option one for now. Hide the progress bar.

I'll add making an EuiMeter component as a later todo on our roadmap. They will be extremely similar and not require too much work to swap out at a later time.

[andreadelrio]

4. Should valueText be created based on value? Instead of asking them for this value again, what if we just spit out ${value}%? Then we'd change the prop to be some sort of enum or boolean.

@cchaos What if they want to show a count instead of a percentage? That's the one reason I see for having valueText be a separate prop. How do you see an enum working?

[kibanamachine]

Preview documentation changes for this PR: https://eui.elastic.co/pr_3661/

[cchaos]

Hmm, good point. We could do something like:

/*
 * If true, will render the percentage, otherwise pass a custom node
 */
valueText: boolean | React.node

Then for the render:

let valueRender;
if (typeof valueText === 'boolean' && valueText) {
  // valueText is a true boolean
  valueRender = `${value}%`
} else if (valueText) {
  // valueText exists
  valueRender = valueText
}

[kibanamachine]

Preview documentation changes for this PR: https://eui.elastic.co/pr_3661/

[andreadelrio]

Implemented the proposed solutions for a11y and for how to handle valueText.

@cchaos I also made some adjustments for Amsterdam. Wondering if the l bar should be even more rounded?

[kibanamachine]

Preview documentation changes for this PR: https://eui.elastic.co/pr_3661/

[myasonik]

EDIT: Made this idea into a follow up issue #3678

---

Trying to think of a way to join the label and value text so there's something more semantic tying them together rather than just proximity...

I'm thinking maybe a <dl> might work? A DOM structure like this maybe:

<dl>
	<dt>Men's clothing</dt>
	<dd>80%</dd>
	<dd aria-hidden="true"><progress /></dd>
</dl>

If we introduced an extra prop for how to render the wrapping element, we could even extend this to better work with a whole list of these like in the demo. Something like:

<dl>
	<EuiProgress as="div" value={20} label={"Men's clothing"} max={100} />
	<EuiProgress as="div" value={20} label={"Women's clothing"} max={100} />
	<EuiProgress as="div" value={20} label={"Women's shoes"} max={100} />
	<EuiProgress as="div" value={20} label={"Men's shoes"} max={100} />
</dl>

Where as changes the wrapping <dl> of each <EuiProgress /> to a <div> so that we can create one list of progress bars, instead of n lists. (Maybe as is too vague and broad given that div is probably the only valid value, but just throwing out an idea.)

[cchaos]

I definitely see the benefit of using the <dl> type of structure but it seems way to prescriptive to do this for every single usage.

• What happens if it's just a single progress bar? <dd aria-hidden> doesn't work by itself.
• What if it's only a single use of the progress with a label? Is it ok to have a list with a single item?

Let's consider just making the elements customizable, or we'll need to create a higher component like EuiListGroup that will create the whole element structure.

With the risk of this PR continually expanding greater, let's table this discussion and put it into an issue for follow up.

[myasonik]

Good points! I made a follow up issue (#3678) for this so that we don't have to block this PR.

[cchaos]

I also made some adjustments for Amsterdam. Wondering if the l bar should be even more rounded?

I think just fully rounding them using border-radius: 100% will work for all sizes.

[cchaos]

A few last comments about the code. I'm going to check out all the permutations/combinations locally now.

[cchaos]

Visually everything looks great, I tried the different sizes, colors, and combos of valueText and label and couldn't seem to break it. However, I'd consider adding the truncate mixin to the valueText as well just in case they throw something long in it like so:

[andreadelrio]

Visually everything looks great, I tried the different sizes, colors, and combos of valueText and label and couldn't seem to break it. However, I'd consider adding the truncate mixin to the valueText as well just in case they throw something long in it like so:

Adding textTruncate to valueText, was truncating it even when there seemed to be enough space for it.

So I defined a flex-basis for label so that its width is limited and it leaves some space for the valueText.

Regarding Amsterdam, border-radius as a percentage doesn't get the desired output. See:

So I set the value in pixels to be high enough to cover the large size (and all the smaller ones too).

[kibanamachine]

Preview documentation changes for this PR: https://eui.elastic.co/pr_3661/

[cchaos]

👍 Looks great! Haha, sorry about the border-radius thing, I think the percentage format is old school thinking... 😊

Just one quick suggestion to your truncation solution, but LGTM

[kibanamachine]

Preview documentation changes for this PR: https://eui.elastic.co/pr_3661/

[kibanamachine]

Preview documentation changes for this PR: https://eui.elastic.co/pr_3661/