Update UX for fielddata percentage / absolute frequency #53966
Conversation
alisonelizabeth
added
Team:Kibana Management
|
Pinging @elastic/es-ui (Team:Elasticsearch UI) |
|
@elasticmachine merge upstream |
| value={[min.value as number, max.value as number]} | ||
| onChange={onFrequencyFilterChange} | ||
| showInput | ||
| fullWidth |
There was a problem hiding this comment.
I think we can supply an append prop to add the "%" sign.
There was a problem hiding this comment.
If append does not work, we should be able to solve it with a 3 columns layout
- Field 1 (
<NumericField field={min} euiProps{{ append: '%' }} />) - Dual range without showing the inputs
- Field 2 (
<NumericField field={max} euiProps{{ append: '%' }} />)
As we have a single source of truth (the min and max hook fields), changing the value on one side will update both components. 😊
There was a problem hiding this comment.
@cjcenizal @sebelga append/prepend are only valid in the input-only version of the EuiDualRange component, which is what design suggested to use. The example included above is using the EuiRange component (I believe).
Here are the updated screens:
This also doesn't take up as much room horizontally, so I also moved the Minimum segment size input inline.
There was a problem hiding this comment.
@alisonelizabeth I was referring to this input from https://elastic.github.io/eui/#/forms/form-controls
There was a problem hiding this comment.
I think these changes look great Alison!
There was a problem hiding this comment.
I agree with @cjcenizal , this looks awesome! 😊
| isInvalid={minIsInvalid} | ||
| fullWidth | ||
| data-test-subj="input" | ||
| controlOnly={true} |
There was a problem hiding this comment.
When I use VoiceOver on OS X to read these inputs, I just get the value in the input followed by "stepper". Is it possible to add internationalized aria-label props for "Minimum absolute frequency" and "Maximum absolute frequency"?
| label={ | ||
| <FormattedMessage | ||
| id="xpack.idxMgmt.mappingsEditor.fielddata.frequencyFilterAbsoluteFieldLabel" | ||
| defaultMessage="Min/max frequency absolute" |
There was a problem hiding this comment.
I wonder if this would be more human-readable if we changed it to "Absolute frequency range". WDYT?
| label={ | ||
| <FormattedMessage | ||
| id="xpack.idxMgmt.mappingsEditor.fielddata.frequencyFilterPercentageFieldLabel" | ||
| defaultMessage="Min/max frequency percentage" |
There was a problem hiding this comment.
Similarly, what do you think of "Percentage-based frequency range"?
| /> | ||
| } | ||
| > | ||
| <EuiDualRange |
There was a problem hiding this comment.
This component has a similar screen-reader accessibility issue. I created elastic/eui#2737 to track this in EUI.
There was a problem hiding this comment.
LGTM! Tested locally and works great. I left a few comments in the code but nothing important.
Have you tried the layout with the minimum segment size on the right as in the design? As the field is not supposed to get many characters (4/5 max), having it full width might not be necessary.
|
|
||
| <EuiSpacer size="s" /> | ||
|
|
||
| <EuiSwitch |
There was a problem hiding this comment.
Can we use the compressed version for the switch? I think that's what Michael had in mind and I think would look nicer.
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
Fixes #53935
Note: I left the controls stacked vertically (differs from mockup) as I believe this was an intentional change done via #52142. Also, the range component does not support appending characters (for
%) AFAICT.