Skip to content

Form elements#121

Merged
lederer merged 33 commits intomasterfrom
feature/sml/form-elements
Jan 9, 2019
Merged

Form elements#121
lederer merged 33 commits intomasterfrom
feature/sml/form-elements

Conversation

@lederer
Copy link
Copy Markdown
Contributor

@lederer lederer commented Jan 2, 2019

Overview

Add critical form components:

  • Label
  • TextInput
    • <input type="text" /> by default. use type prop to change to password, email, etc
  • Textarea
  • Select
    • supports multiple via native <select multiple />
  • Radio
    • controlled or uncontrolled
  • Checkbox
    • controlled or uncontrolled. supports indeterminate.
  • FileInput
  • ColorInput
  • Field
    • wraps a child form component in a <label> and accepts optional hint, alert, direction, and other props
  • ToggleField
    • like Field but wraps related Radios or Checkboxes

These aren't fully tested in terms of handling events and being composed into complex components and layouts, but they should suffice as a foundational set of form components we can begin using now and can improve upon along the way.

Checklist

  • Relevant documentation pages have been created or updated
  • Description of PR is in an appropriate section of the changelog and grouped with similar changes if possible

Are there any of the following in this PR?

  • Changes to the prop names or types of existing components
  • Changes in intended behavior of existing component props
  • Changes in the theme file's structure
  • A required version bump to a non-dev dependency of the project

If one of the above is checked...

  • information or guidance around needed changes for consumers of this library has been added to the changelog under Upgrade Instructions

Demo

image

image

image

image

image

image

image

image

image

image

image

image

Notes

Punted to separate issues:

  • Fancy custom select dropdown
  • RangeInput
  • Date/time inputs
    • <input type="{date, time, datetime-local, month, week}" />

Closes #30

Copy link
Copy Markdown
Contributor

@designmatty designmatty left a comment

Choose a reason for hiding this comment

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

Some high level issues that I discovered that I'd like to get fixes for prior to digging into the code:

All components that allow text entry (Textfield, Textarea, etc):

  • If you provide a value prop, it prevents the user from overriding the value when they attempt to type into the field. This prop is useful, for example, in user settings fields where we display a user editable field for name/email/etc with their previously entered values but allow them to change them.

Colorinput:

  • Same issue as above: by providing the value prop, it prevents the user from overriding

Radio/Checkbox:

  • Accessibility issue: no focus state

Togglefield:

  • clicking label does not focus on input
    • This is due to having nested labels
  • Accessibility issue: no focus state

Field:

  • clicking label does not focus on input
    • This is due to having nested labels

@lederer
Copy link
Copy Markdown
Contributor Author

lederer commented Jan 2, 2019

On it!

@lederer
Copy link
Copy Markdown
Contributor Author

lederer commented Jan 2, 2019

Pushed a fix for the Radio/Checkbox focus and Field/ToggleField label click issues.

TextInput is working as intended, insofar as setting the value prop turns the component into a controlled component. When setting value, the containing component should also pass a handler to the onChange prop that's responsible for updating the value prop according to the container's custom logic.

That said, passing a default value to an uncontrolled component is a great use case, one that Blueprint handles nicely via the defaultValue prop. I'll get that running, then signal when it's ready for another look.

@designmatty
Copy link
Copy Markdown
Contributor

@lederer it looks like defaultValue works for free and is a built-in function of react. I think the only thing worth doing at this point, is documenting it in our props table.

@lederer
Copy link
Copy Markdown
Contributor Author

lederer commented Jan 3, 2019

Yay!

Documented defaultValue in TextInput. It's inherited by Textarea and ColorInput (and FileInput).

Also wired up and documented defaultChecked in Radio and Checkbox.

Ready for another look.

Copy link
Copy Markdown
Contributor

@designmatty designmatty left a comment

Choose a reason for hiding this comment

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

In textInput and select you've defined a font propType which doesn't do anything. Did you mean for this to be styled-system fontFamily propType?

@lederer
Copy link
Copy Markdown
Contributor Author

lederer commented Jan 7, 2019

Doh. Yes, I think that should be fontFamily. The default inherit value may be redundant with our global styles, but worth keeping I think.

@designmatty
Copy link
Copy Markdown
Contributor

I have one suggestion for an enhancement before merging this in:
Our alerts for the field and toggleField should recieve an intent. For instance, on a password field, you could indicate the stength of the password using an alert. Bad password = red, okay password = orange, and good password = green.

So once we implement that one enhancement and the fix forfontFamily, this PR is good to go.

@lederer lederer force-pushed the feature/sml/form-elements branch from 6198d8f to 1e2d8a7 Compare January 8, 2019 18:10
@lederer
Copy link
Copy Markdown
Contributor Author

lederer commented Jan 8, 2019

Made those adjustments.

Example of alertIntent={Intent.SUCCESS}:
image

@designmatty
Copy link
Copy Markdown
Contributor

designmatty commented Jan 9, 2019

This looks good to go

@lederer lederer merged commit b9a1326 into master Jan 9, 2019
@lederer lederer deleted the feature/sml/form-elements branch January 9, 2019 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants