search

GriddleGriddle / Griddle

Simple Grid Component written in React
http://griddlegriddle.github.io/Griddle/
MIT License
2.5k stars 378 forks source link

Update ColumnChooser #730

Open ahopkins opened 7 years ago

ahopkins commented 7 years ago

Griddle major version

v1

Changes proposed in this pull request

Add id attribute to the input

Why these changes are made

To allow for clicking of label to toggle the checkbox

Are there tests?

No, it is a single HTML attribute

dahlbyk commented 7 years ago

To allow for clicking of label to toggle the checkbox

Are you sure this is necessary? <label><input /> Test</label> should already work; id+for is only necessary if the input is outside the label (W3C).

ahopkins commented 7 years ago

It was not working for me at all in Firefox Dev Version. I'll send you the specifics of the browser.

On Fri, Aug 25, 2017, 00:00 Keith Dahlby notifications@github.com wrote:

To allow for clicking of label to toggle the checkbox

Are you sure this is necessary? should already work; id+for is only necessary if the input is outside the label ( W3C https://www.w3.org/WAI/tutorials/forms/labels/#associating-labels-implicitly ).

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/GriddleGriddle/Griddle/pull/730#issuecomment-324756433, or mute the thread https://github.com/notifications/unsubscribe-auth/AAKJfbEUOS_n_NA-m3hzDYDj9RjTwsOlks5sbeSHgaJpZM4PBETV .

dahlbyk commented 7 years ago

It was not working for me at all in Firefox Dev Version. I'll send you the specifics of the browser.

No need; I've confirmed what you're seeing. I think the better fix would be to remove htmlFor from both labels.