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

Form Label uses pointer #823

Closed
w33ble opened this issue May 11, 2018 · 10 comments
Closed

Form Label uses pointer #823

w33ble opened this issue May 11, 2018 · 10 comments

Comments

@w33ble
Copy link
Contributor

w33ble commented May 11, 2018

Is there a reason that eui form labels use a pointer?

There are no click handlers in the component. Clicking them is futile.

may-11-2018 14-10-16

@chandlerprall
Copy link
Contributor

chandlerprall commented May 11, 2018

Most labels are connected to the associated inputs (browsers have this functionality built in based on the label's for and the input's id attributes). In most uses, the label is clickable and will simply shift focus to its input.

I agree that the cursor should not be a pointer in your situation.

@snide maybe move the cursor rule to a .euiFormLabel[for] {} block so it only applies when the label has a for attribute?

@w33ble
Copy link
Contributor Author

w33ble commented May 11, 2018

Seems like some kind of parent/child component to handle the click linking would work here, so you could still link clicking on the label with focus a given input.

I'd be fine with just being able to opt out of the pointer cursor via an arg too.

@snide
Copy link
Contributor

snide commented May 11, 2018

@chandlerprall That would probably work.

Labels should always be clickable and focus the input / element you are applying it towards. Are you applying correct id/for attributes in your markup @w33ble?

Also, those don't look like our labels fwiw. What does your rainbow item do? Does it accept focus? I assume those are separate buttons so they can be accessible? You'll need something to map the context.

image

@w33ble
Copy link
Contributor Author

w33ble commented May 11, 2018

Are you applying correct id/for attributes in your markup @w33ble?

Probably not. Are there docs for this anywhere? I couldn't find them on the docs site, so I'm not actually sure how to use the component correctly. Also, the form control for the label isn't a real form control, so it wouldn't do anything natively, we'd have to wire it up manually.

@snide
Copy link
Contributor

snide commented May 11, 2018

@w33ble The EuiFormRow stuff does the parent/child auto ids you mention. It'll require your inner item to be able to accept focus though. I'm gonna bet that widget doesn't and isn't accessible so might be something you want to look into separately.

https://elastic.github.io/eui/#/forms/form-layouts

Use the EuiFormRow component to easily associate form components with labels, help text, and error text. Use the EuiForm component to group EuiFormRows.

Beyond that, id/for is standard HTML. I don't know that we'd doc it.

I'll make the CSS fix @chandlerprall suggested though. I think that's a good solution for the cursor issue.

@w33ble
Copy link
Contributor Author

w33ble commented May 11, 2018

It looks like somehow the for attribute is being set automatically, perhaps from some parent component (EDIT: yup)? So using the selector, while a step in the right direction, doesn't really solve the issue for us.

screenshot 2018-05-11 15 10 59

I'm certain we're using it wrong, but I don't know what we need to do to make it right.

@snide
Copy link
Contributor

snide commented May 11, 2018

Sec, I'll build you a quick demo.

@snide
Copy link
Contributor

snide commented May 11, 2018

https://codesandbox.io/s/8yz3npzool

You'd do it this way if you wanted to avoid the visual glitch. Basically instead of using the magic labeling, you can just individually construct it from the plain components. It requires you to wire up your for/to or aria-describedby properties on your own though.

@snide
Copy link
Contributor

snide commented May 11, 2018

And of course manage the state of the focus on your own if you wanted the labels to turn blue.

@snide
Copy link
Contributor

snide commented May 14, 2018

Closing this in place of #826 which better details the issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants