Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix Validated Components #340

Merged
merged 8 commits into from
Oct 16, 2019
Merged

Fix Validated Components #340

merged 8 commits into from
Oct 16, 2019

Conversation

alexpaxton
Copy link
Contributor

@alexpaxton alexpaxton commented Oct 16, 2019

Closes #339

Turns out the bug was caused by the hexcode validator function always returning a string when it was expecting a null value to determine if is valid, hence it always returning invalid

Changes

  • Change typing of FormValidationElement to expect string instead of any
    • Turns out the component is exclusively used to validate text inputs
  • Remove state from ColorPicker and FormValidationElement
    • Should make these work like controlled components
  • Create a type for ValidationFunction to enforce consistency across these components (and potentially more)
    • Validation functions should return a string (error) or null (valid)
  • Add validationFunc prop to ColorPicker
    • Has a hexcode validator by default
    • User can pass in their own function
  • Updated stories to include inputs for testing purposes

Checklist

Check all that apply

  • Updated documentation to reflect changes
  • Added entry to top of Changelog with link to PR (not issue)
  • Tests pass
  • Peer reviewed and approved
  • Signed CLA (if not already signed)

@alexpaxton alexpaxton requested a review from drdelambre October 16, 2019 21:09
Copy link

@drdelambre drdelambre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i like where the code is moving, but i dont understand how the chaining of validation handles.

@@ -110,9 +111,9 @@ const ConfirmationContents: FunctionComponent<{
returnValue?: any
onConfirm: (returnValue?: any) => void
confirmationLabel: string
testID?: string
testID: string

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😲

},
ref
) => {
const [errorMessage, setErrorMessage] = useState('')
const errorMessage = validationFunc(color)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this be more of a validateHexCode(color) && validationFunc(color) relationship? so that validating that a color matches a palette map somewhere doesnt mean you have to rewrite validateHexCode?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made validationFunc to be validateHexCode by default, but in case we ever want to support rgb values or named colors a different validation function can be passed in

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


setErrorMessage(newErrorMessage)
onChange(cleanedValue, inputStatus)
onChange(e.target.value, inputStatus)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why no more input cleaning here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose I could trim out space characters at the very least

@alexpaxton
Copy link
Contributor Author

@drdelambre can you clarify "Chaining of validation"?

@drdelambre
Copy link

like.. whats the difference between a text input, color hex input, and a "only shades of grey" input? is the thinking that the last input is going to be custom validation function + hex validation function + text validation function, or is it custom validation function implements (hex validation function + text validation function)? is the idea that v1.0 is a transition from the former to the later?

Copy link

@drdelambre drdelambre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

offlined the conversation and we're aiming for max flexibility

@alexpaxton alexpaxton merged commit c3ce26d into master Oct 16, 2019
@alexpaxton alexpaxton deleted the fix/validated-components branch October 16, 2019 23:03
@alexpaxton alexpaxton mentioned this pull request Oct 16, 2019
5 tasks
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Color picker always invalid
2 participants