This repository has been archived by the owner on Jun 24, 2021. It is now read-only.
Web/refactor/input usage #60
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
isSubmit is used to specify if the button is a submit button or not.
isSubmit is used to specify if the button is a submit button or not.
We can determine if it's in a form or not by the name prop
bobheadxi
approved these changes
Aug 18, 2018
| @@ -246,7 +246,7 @@ describe('Hacker component', () => { | |||
| }); | |||
|
|
|||
| it('page one is rendered', () => { | |||
| expect(wrapper.find('div#hacker-application-page-1')).toHaveLength(1); | |||
| expect(wrapper.find('form#hacker-application-page-1')).toHaveLength(1); | |||
There was a problem hiding this comment.
@mingyokim an example of the potentially-overly-specific-tests i talked about the other day
There was a problem hiding this comment.
yeah, i get it now. i'll try to avoid these patterns next time
mingyokim
suggested changes
Aug 18, 2018
| disabled={disabled} | ||
| className={`primary ${className}`}> | ||
| className={`primary ${className}`} | ||
| form={name}> |
There was a problem hiding this comment.
do we really need form here? It's not submitting even when the type is submit if form is defined
There was a problem hiding this comment.
Oops, form was supposed to equal the form-id but I made it the form-name. Fixed by fd5ad1c. The commit also removes this attribute altogether since it's only really useful in the extreme case where you want to associate a button with a form when it's outside the <form> tags.
3 tasks
The use case for this is pretty rare
It doesn't pick up type defined through defaultProps (since it's dynamic), so we have to set this to warn or disable it altogether.
chamkank
commented
Aug 30, 2018
| @@ -22,6 +22,7 @@ | |||
| "react/prefer-stateless-function": 0, | |||
| "react/no-did-mount-set-state": 0, | |||
| "react/jsx-one-expression-per-line": 0, | |||
| "react/button-has-type": 1, | |||
There was a problem hiding this comment.
mingyokim
approved these changes
Aug 31, 2018
|
Thanks guys! |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
🎟️ Ticket(s): Closes #57
👷 Changes
Just a small refactor of our input components to use the same conventions before we move forward with the rest of the components. Here are the changes:
idfor each component (just the inputnameinstead).typefield toPrimaryButtonandSecondaryButton(see notes).💭 Notes
ForYou can also explicitly define thePrimaryButton,SecondaryButton, andProgressGroup, thenameis optional. For the buttons, the button type becomessubmitif you provide a value forname. If you don't provide anamevalue, the button type defaults tobutton.type(tosubmit,button, orresetwith the default beingbutton) ofPrimaryButtonandSecondaryButton.