Tie field to label without explicit id, and to supporting elements #130

zinckiwi · 2017-11-09T10:13:39Z

generate a random id for the field if necessary, similar to what the docs do
associate help and error text with the field via aria-describedby attributes

…a aria attributes

pugnascotia

Changes look good, but can we get some test coverage of the aria field getting injected?

zinckiwi · 2017-11-09T10:48:05Z

Yes, would be good to add. Should have mentioned this isn't intended to be merge-ready just yet.

Another issue is that all requisite generation is repeated each render, so depending on rerender frequency there could be a lot of churn. We address this in Cloud's Field component by keeping generated ids in state, reusing them between render cycles. But before I go to that effort if we think it's warranted, figured I'd get some approval in principle.

snide · 2017-11-09T15:20:26Z

FYI I think there's a service around that can do this already. https://github.com/elastic/eui/blob/master/src/services/accessibility/html_id_generator.js

@cjcenizal pulled it in a little later on, which is why the docs didn't use it as much. He'll know more about it then me.

zinckiwi · 2017-11-09T15:39:39Z

Even better. Is that globally mocked for tests by chance?

cjcenizal · 2017-11-09T15:55:57Z

@zinckiwi Not sure what you mean by globally mocked? It's not global, so you'll need to import it into your test to use it.

zinckiwi · 2017-11-09T16:00:31Z

But we can't snapshot a randomly generated id -- it will need to return some deterministic value in a test, surely?

cjcenizal · 2017-11-09T16:06:20Z

Ah I see. I think the usage of htmlIdGenerator is kind of confusing. To be honest I borrowed it from X-Pack and have never used it myself. Here's how it works:

// Instantiate an ID generator with a prefix.
const idGenerator = htmlIdGenerator('loginForm');

// Then you can create "namespaced" IDs by calling the generator with a suffix.
const userNameId = idGenerator('userName');
const passwordId = idGenerator('password');
// "loginForm_userName" and "loginForm_password" respectively.

// You can use the dynamically-created UID feature by omitting the suffix argument.
const emailId = idGenerator();
// "loginForm_${random UID}"

I think the whole point behind this service is to allow you to generate unique IDs for repetitive, dynamically-generated forms. Where you'll have several inputs which repeat but which will still need unique IDs. Does this help at all?

zinckiwi · 2017-11-09T16:21:33Z

Ah, I understand. I'm getting at something different here. Labels need to be tied to inputs, and inputs need to be tied to related descriptive elements (i.e. help text and errors). That must be done through ids. Personally, I don't have any use for ids outside of this context.

Given all that, I like to generate statistically almost-certanly-unique ids on the fly. That goes extra for a component-based system like React, since you can never guarantee uniqueness of ids that you specify by hand attempting to be semantic (e.g. id='email-field') since you can't know for sure how/where a component will be used in the future.

cjcenizal · 2017-11-09T16:28:53Z

I like to generate statistically almost-certanly-unique ids on the fly

Makes sense to me. Does the last example in the code snippet address this?

const emailId = idGenerator();

cjcenizal · 2017-11-09T16:30:17Z

Oh, and I'm sorry I should also clarify that the IDs in my example are intended to be assigned to labels, form elements, and descriptive text elements, which is what you are trying to do.

zinckiwi · 2017-11-09T16:30:32Z

Yep, should do. I mean, they're both probably the same thing perhaps differing by length -- my bigger concern is the churn so if you have thoughts on that I'm all ears!

cjcenizal · 2017-11-09T16:33:16Z

Well, now that I've actually read the code in this PR I am able to provide better suggestions... go figure. 🤕 I jumped into the middle of the convo so I missed the overarching context. Is there any reason we shouldn't just make id required? Offload the burden to the consumer?

cjcenizal · 2017-11-09T16:36:35Z

src/components/form/form_row/form_row.js

      hasEmptyLabelSpace,
      fullWidth,
      className,
      ...rest
    } = this.props;

+    const id = rest.id


If we make id required, the consumer can use the generator to create an id with a unique suffix.

cjcenizal · 2017-11-09T16:37:06Z

src/components/form/form_row/form_row.js

@@ -60,7 +65,7 @@ export class EuiFormRow extends Component {

    if (helpText) {
      optionalHelpText = (
-        <EuiFormHelpText className="euiFormRow__text">
+        <EuiFormHelpText id={makeId()} className="euiFormRow__text">


We could then append the string "help" to the id that was provided to create this id.

cjcenizal · 2017-11-09T16:37:27Z

src/components/form/form_row/form_row.js

@@ -71,7 +76,7 @@ export class EuiFormRow extends Component {
    if (error) {
      const errorTexts = Array.isArray(error) ? error : [error];
      optionalErrors = errorTexts.map(error => (
-        <EuiFormErrorText key={error} className="euiFormRow__text">
+        <EuiFormErrorText key={error} id={makeId()} className="euiFormRow__text">


We could do something similar here by appending "error" and index to create IDs for the errors.

zinckiwi · 2017-11-09T16:38:34Z

Well, that's what I meant about manually specifying them... in the React world at least I don't think they serve any purpose beyond this one, so offloading that burden truly is that -- and multiplying the burden by however many customers there are. And unless you only ever write forms at a "page" level, you never know if you'll have id collisions with other components that happen to be rendered at the same time. (Unless you write very specific ids that are namespaced appropriately, etc., but q.v. burden.)

zinckiwi · 2017-11-09T16:52:16Z

Recapping quick zoom chat with CJ:

htmlIdGenerator is a better solution than mine for creating ids for the help text, errors etc. -- the constituent parts within a field. We can use it with the main field id as the stem, so a1b2c23_help, a1b2c3_error_1, a1b2c3_error_2, etc.
CJ can go either way on automatically generating that main field id at runtime if one isn't explicitly provided, but I regard explicit ids as an antipattern in a component-based framework where ids must be unique on the page. So I'll keep that in and let the peanut gallery make the call :)

zinckiwi · 2017-11-10T10:27:18Z

Okay, updated. Still using the local id generator since a) we don't need a whole uuid and b) The prefixes are either hard-coded (help) or hard-coded and sequential (error-n). End result is this:

pugnascotia

LGTM

cjcenizal

Looks great man! I had one minor suggestion.

cjcenizal · 2017-11-10T14:39:58Z

src/components/form/form_row/make_id.js

@@ -0,0 +1,3 @@
+export default function makeId() {


How about a comment for posterity?

// Generate statistically almost-certainly-unique `id`s for associating form inputs with their labels // and other descriptive text elements.

…e-field-parts

…e-field-parts # Conflicts: # CHANGELOG.md

Tie field to label without explicit id, and to supporting elements vi…

4c64a20

…a aria attributes

zinckiwi requested review from snide, bevacqua, cjcenizal and pugnascotia November 9, 2017 10:13

Don’t need a map

e498857

pugnascotia reviewed Nov 9, 2017

View reviewed changes

snide added the work in progress label Nov 9, 2017

cjcenizal reviewed Nov 9, 2017

View reviewed changes

Tweak id generation and add test

5edc98a

pugnascotia approved these changes Nov 10, 2017

View reviewed changes

cjcenizal approved these changes Nov 10, 2017

View reviewed changes

Add comment about make_id

459fb8d

snide approved these changes Nov 10, 2017

View reviewed changes

zinckiwi added 2 commits November 10, 2017 17:26

Merge commit '169e1066c4e86ad1f1258957588371e48929fcd0' into associat…

e69149a

…e-field-parts

Add changelog entries

626f88d

zinckiwi added 2 commits November 10, 2017 17:40

Tweak changelog entries

795201a

Merge commit '98010b5935070178e0c9578f4991c8e472beaa7d' into associat…

ca41f4e

…e-field-parts # Conflicts: # CHANGELOG.md

zinckiwi merged commit d9bad70 into elastic:master Nov 10, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tie field to label without explicit id, and to supporting elements #130

Tie field to label without explicit id, and to supporting elements #130

zinckiwi commented Nov 9, 2017

pugnascotia left a comment

zinckiwi commented Nov 9, 2017

snide commented Nov 9, 2017 •

edited

Loading

zinckiwi commented Nov 9, 2017

cjcenizal commented Nov 9, 2017

zinckiwi commented Nov 9, 2017

cjcenizal commented Nov 9, 2017 •

edited

Loading

zinckiwi commented Nov 9, 2017

cjcenizal commented Nov 9, 2017

cjcenizal commented Nov 9, 2017

zinckiwi commented Nov 9, 2017

cjcenizal commented Nov 9, 2017

cjcenizal Nov 9, 2017

cjcenizal Nov 9, 2017

cjcenizal Nov 9, 2017

zinckiwi commented Nov 9, 2017

zinckiwi commented Nov 9, 2017

zinckiwi commented Nov 10, 2017

pugnascotia left a comment

cjcenizal left a comment

cjcenizal Nov 10, 2017

Tie field to label without explicit id, and to supporting elements #130

Tie field to label without explicit id, and to supporting elements #130

Conversation

zinckiwi commented Nov 9, 2017

pugnascotia left a comment

Choose a reason for hiding this comment

zinckiwi commented Nov 9, 2017

snide commented Nov 9, 2017 • edited Loading

zinckiwi commented Nov 9, 2017

cjcenizal commented Nov 9, 2017

zinckiwi commented Nov 9, 2017

cjcenizal commented Nov 9, 2017 • edited Loading

zinckiwi commented Nov 9, 2017

cjcenizal commented Nov 9, 2017

cjcenizal commented Nov 9, 2017

zinckiwi commented Nov 9, 2017

cjcenizal commented Nov 9, 2017

cjcenizal Nov 9, 2017

Choose a reason for hiding this comment

cjcenizal Nov 9, 2017

Choose a reason for hiding this comment

cjcenizal Nov 9, 2017

Choose a reason for hiding this comment

zinckiwi commented Nov 9, 2017

zinckiwi commented Nov 9, 2017

zinckiwi commented Nov 10, 2017

pugnascotia left a comment

Choose a reason for hiding this comment

cjcenizal left a comment

Choose a reason for hiding this comment

cjcenizal Nov 10, 2017

Choose a reason for hiding this comment

snide commented Nov 9, 2017 •

edited

Loading

cjcenizal commented Nov 9, 2017 •

edited

Loading