-
Notifications
You must be signed in to change notification settings - Fork 4k
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
fix(Form): pass shorthand ids to the label's htmlFor prop #1517
fix(Form): pass shorthand ids to the label's htmlFor prop #1517
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1517 +/- ##
=======================================
Coverage 99.74% 99.74%
=======================================
Files 141 141
Lines 2396 2396
=======================================
Hits 2390 2390
Misses 6 6
Continue to review full report at Codecov.
|
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.
Docs look great, thanks! I've left a couple comments.
<Form.Input label='First name' placeholder='First name' /> | ||
<Form.Input label='Last name' placeholder='Last name' /> | ||
<Form.Input id='form-input-shorthand-first-name' label='First name' placeholder='First name' /> | ||
<Form.Input id='form-input-shorthand-last-name' label='Last name' placeholder='Last name' /> |
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.
I'd like to have two dedicated examples for this in the docs under one heading. One example showing the usage with Form.Field
and another showing usage with Form
shorthand subcomponents.
First, create two example files in docs/app/Examples/collections/Form/Shorthand
, probably:
FormExampleFieldControlId.js
showingForm.Field
use with the shorthandcontrol
prop.FormExampleSubcomponentId.js
showing usage of one or moreForm
subcomponents, i.e.Form.Input
.
Then, add these files to the index.js
file in the same directory. You link examples with the ComponentExample
component, you'll see several in the index.js
file. In order to get two examples to show under one title, only add a title
to the first example but descriptions
to both:
<ComponentExample
title='Accessible lables'
description='Adding an id to a shorthand Form.Field adds a matching htmlFor prop to the label.'
examplePath='collections/Form/Shorthand/FormExampleFieldControlId'
/>
<ComponentExample
description='Adding an id to a Form subcomponent adds a matching htmlFor prop to the label.'
examplePath='collections/Form/Shorthand/FormExampleSubcomponentId'
/>
src/collections/Form/FormField.js
Outdated
@@ -93,7 +93,7 @@ function FormField(props) { | |||
|
|||
return ( | |||
<ElementType className={classes}> | |||
{createHTMLLabel(label)} | |||
{createHTMLLabel(label, { htmlFor: controlProps && controlProps.id })} |
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.
We'll need a test that asserts the htmlFor
prop is added when an id
is passed to FormField
. You can find tests in test/specs/collections/Form/FormField-test.js
.
Let's also add this test to each subcomponent usage (Form.Input
, etc). These tests would go in test/specs/collections/Form/Form-test.js
.
Because this test will be repeated several times, the test itself should be added only once to commonTests.js
. You'll see several examples of how to write a common test there. Just add it to the bottom. You'll see several examples of how to use a common test in the Form tests I've noted above.
I've noticed a couple typos and mixed up references in the Form docs, I'll be pushing those fixes to this branch as well as they are pretty minor. |
src/lib/htmlInputPropsUtils.js
Outdated
@@ -6,7 +6,7 @@ export const htmlInputAttrs = [ | |||
|
|||
// LIMITED HTML PROPS | |||
'autoCapitalize', 'autoComplete', 'autoFocus', 'checked', 'form', 'max', 'maxLength', 'min', 'multiple', | |||
'name', 'pattern', 'placeholder', 'readOnly', 'required', 'step', 'type', 'value', | |||
'name', 'pattern', 'placeholder', 'readOnly', 'required', 'step', 'type', 'value', 'id', |
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.
Please sort props in alphabetical order.
Ok guys, I will do that. |
Given the number of changed lines and the lack of visible characters changed, I'm betting your IDE is changing the line endings of each file edited. Are you on Windows by chance? |
Usually in Mac but sometimes in Windows. The commit changed more files that suppose? I check the files when I push and seems all ok. |
The files are OK, it is the number of changed lines: And the nature of the changes. No visible characters have changed, it seems it is likely the line endings: Could you confirm your config is in alignment with these instructions and run the commands from here: https://help.github.com/articles/dealing-with-line-endings? Let's see if it is indeed the line endings. |
It's not the line endings. I checked in the instructions. And now? I need to do something more to the pull request be accepted? |
@@ -16,8 +16,8 @@ class FormExampleSubComponentControl extends Component { | |||
return ( | |||
<Form> | |||
<Form.Group widths='equal'> | |||
<Form.Input label='First name' placeholder='First name' /> | |||
<Form.Input label='Last name' placeholder='Last name' /> | |||
<Form.Input id='form-input-shorthand-first-name' label='First name' placeholder='First name' /> |
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.
Let's remove these now that we other examples.
import { Form, Input } from 'semantic-ui-react' | ||
|
||
class FormExampleSubcomponentId extends Component { | ||
state = {} |
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.
This should be a stateless component:
const FormExampleSubcomponentId = () => (
<Form>
...
</Form>
)
import { Form, Input, TextArea, Button } from 'semantic-ui-react' | ||
|
||
class FormExampleFieldControlId extends Component { | ||
state = {} |
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.
This should be a stateless component:
const FormExampleFieldControlId = () => (
<Form>
...
</Form>
)
const { requiredProps = {} } = options | ||
describe(`${propKey} (common)`, () => { | ||
assertRequired(Component, 'a `Component`') | ||
it(`adds ${requiredProps.control} Id to Label's HtmlFor prop`, () => { |
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.
We'll need generalize this test a bit more. It shouldn't know which component it is testing. We should remove the control
awareness.
We will then be able to use it in the other Form*
component tests:
labelImplementsHtmlForProp(FormFieldButton) // FormFieldButton-test.js
labelImplementsHtmlForProp(FormFieldCheckbox) // FormFieldCheckbox-test.js
labelImplementsHtmlForProp(FormFieldDropdown) // FormFieldDropdown-test.js
labelImplementsHtmlForProp(FormFieldGroup) // FormFieldGroup-test.js
labelImplementsHtmlForProp(FormFieldInput) // FormFieldInput-test.js
labelImplementsHtmlForProp(FormFieldRadio) // FormFieldRadio-test.js
labelImplementsHtmlForProp(FormFieldSelect) // FormFieldSelect-test.js
labelImplementsHtmlForProp(FormFieldTextarea) // FormFieldTextarea-test.js
Suggest we don't take any arguments as this test will require the id
of the top level component to present no the label
subcomponent after it is rendered.
I've rebased this to master and fixed conflicts. I also pushed some line ending fixes, I left a few final comments that should wrap this one up! |
…s): convert to stateless components
…ntsHtmlForProp test and call each form type on corresponding test file
For "Checkbox", "Radio" and "Group" the "htmlFor" is not added, so they have not this new test. To implement this in this components I think we need to do different from what i do to the others. |
So, any news about this? |
…React into accessibility/label-click-for-input-element # Conflicts: # src/lib/htmlInputPropsUtils.js
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.
Changes look great, thanks!
I've resolved conflicts and will merge on pass. |
@levithomason I wan't push some changes, please wait 5 minutes |
…/github.com/pedrocostadev/Semantic-UI-React into accessibility/label-click-for-input-element
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.
Looks awesome, @pedrocostadev thanks for PR
We can merge on pass
src/collections/Form/FormField.js
Outdated
{createHTMLLabel(label, { htmlFor: controlProps && controlProps.id })} | ||
{createHTMLLabel(label, { defaultProps: { | ||
htmlFor: _.get(controlProps, 'id') }, | ||
})} |
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.
Factories where updated earlier
labelNode.should.have.prop('htmlFor', idToTest) | ||
|
||
wrapper.should.to.have.descendants(`#${id}`) | ||
labelNode.should.have.prop('htmlFor', id) |
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.
Updated to use BDD style
I'm glad to help. Let's go to another :-) |
💥 |
Released in |
Thanks all - I am glad this was released! |
Added new prop id to Form.Input's component and use it also for htmlFor label. Updated the docs example to show results.
This is my first step here so, if you need something more to merge, just tell me please :-)
Resolve #980