-
Notifications
You must be signed in to change notification settings - Fork 842
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
Add labelAppend
prop to EuiFormRow
and allow EuiFormLabel
to be a <legend>
#1613
Conversation
</EuiFormLabel> | ||
// Outer div ensures the label is inline-block (only takes up as much room as it needs) | ||
<div> | ||
<EuiFormLabel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having been interacting with the forms and especially combo boxes for a while now, it's been frustrating me that the white space to the right of form label text is also clickable. Like when I'm trying to close the combo box's dropdown by clicking outside of it, I think I'm click on a non-object, but really it's the form label that being block displayed.
I checked out the default display behavior of html label
elements, and they're inline
. I think we should lean on the default. So I set the .euiFormLabel
to be inline-block
(so it'd get margins) but because EuiFormRow is displayed as flex
with direction of column
that forces everything to be display block
.
This wrapping <div>
forces the div itself to be block but allowing it's contents to be whatever.
The other options to remove these lines, but I don't know the implication down the line...
eui/src/components/form/form_row/_form_row.scss
Lines 1 to 7 in dae4fe7
/** | |
* 1. Coerce inline form elements to behave as block-level elements. | |
* 2. For inline forms, we need to add margin if the label doesn't exist. | |
*/ | |
.euiFormRow { | |
display: flex; /* 1 */ | |
flex-direction: column; /* 1 */ |
This looks great, Caroline! |
This is ready for review. |
className={classes} | ||
{...rest} | ||
id={`${id}-row`} | ||
aria-labelledby={labelID} // Only renders a string if label type is 'legend' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I was astounded that when using fieldset and label, the screenreader didn't automatically announce the screenreader. Figured, I'd just add this aria-labelledby
stuff and it works. I now realize that it's because the legend
wasn't a direct descendent of fieldset
. If I remove the wrapping div as mentioned above, it works without the aria stuff....
Just an FYI really...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, one small change requested to help clarify label vs legend typing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried pretty hard to find something here, but it all looks and works well. I spent most of my time testing accessibility, thinking this might have effected how the screen readers handled bits, but after scanning all the form types and their sibling pages I couldn't find anything.
The legend / label stuff is nice. This isn't tied to this PR, but we should likely use a described by on the checkboxes to read out the legend. I hacked this into my inspector and it seemed to work the way I expected.
Yeah it's not really possible to do that right now at the EuiFormRow level since it doesn't know what it's form inputs are. |
@cchaos Yep, no worries. I figured as much. |
This also means EuiFormRow can render as a `fieldset` instead of simple `div`
Added id and `aria-labelledby` for fieldset/legend combos
8bc2199
to
62bee3d
Compare
Add
labelAppend
prop toEuiFormRow
To help with instances where consumers need to add content to the form row's label (but really shouldn't live inside the label like a clickable element), they can now use
labelAppend
which accepts any node, and just displays it in aspaceBetween
style flex group with the label.Example:
renders:
Note:
I purposefully didn't want to trap us into a situation where the prop only allowed links or other granular content, so I just allowed any node. This does make creating similarly font-sized links a little more tedious.
Allow
EuiFormLabel
to be a<legend>
Fixes #1136 so that situations (like checkbox groups) where the label should really be acting like a fieldset legend can. It just allows you to set the new
type
prop to either'label' | 'legend'
.Subsequently, I added
labelType
which accepts the same possibilities to the EuiFormRow which will pass the prop correctly to the label but also use thefieldset
element if using alegend
type.Example
renders the same visually, but the html is
Also
Did a quick style alignment between the table headers and form labels. The table headers are slightly less bold, while the form labels are darker.
cc @cristina-eleonora
Checklist
[ ] This was checked for breaking changes and labeled appropriately[ ] This required updates to Framer X components