Skip to content
This repository was archived by the owner on Mar 3, 2020. It is now read-only.

Mibs/feature form#15

Open
mibsbalsante wants to merge 270 commits intomasterfrom
mibs/feature-form
Open

Mibs/feature form#15
mibsbalsante wants to merge 270 commits intomasterfrom
mibs/feature-form

Conversation

@mibsbalsante
Copy link
Copy Markdown
Contributor

What is new?

  • Stateless inputs
  • Input validators
  • Form component

Mibs Balsante added 30 commits July 19, 2018 16:36
that is composed of a `label` passed as `children` prop, a `Input` component and a error message
passed as `errorMessage` prop
…t-next-boilerplate into mibs/feature-storybook
that is composed of a `label` passed as `children` prop, a `Input` component and a error message
passed as `errorMessage` prop
Mibs Balsante added 20 commits July 30, 2018 11:42
Copy link
Copy Markdown
Member

@douglasgimli douglasgimli left a comment

Choose a reason for hiding this comment

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

Awesome work, a few small inline comments :) 👏 👏 👏


const Checkbox = props => {
const { className, isChecked } = props
const inputClasses = classNames({
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

When using classnames with fixed and dynamic fields you can use it like this:
classNames(className, styles.input, { [styles.checked]: isChecked })
No need to add the true to the fixed items.

})

return (
<label className={styles.checkbox} role="checkbox" aria-checked={isChecked}>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is the label necessary? It should be used to add labels to fields, but there's no label here.


Checkbox.propTypes = {
className: PropTypes.string,
id: PropTypes.string.isRequired,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You don't use this id.

value,
onChange: handleChange,
}
return multiline ? <textarea {...props} /> : <input {...props} type={type} checked={checked} />
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👏

color: #323232;
border: 1px solid #979797;
border-radius: 3px;
padding: .6rem .8rem;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

rem 🤣

const labelProps = { htmlFor: field }

const labelComponent =
label && label.type ? (
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why did you clone the element?

isSubmitValid: {},
}
}
onSubmit = ev => {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd like more to use the explicit approach, there's no need to use the abbreviation, why not use event?

styles['font-default'] = "'Source Sans Pro', sans-serif"
styles['font-header'] = "'Poppins', sans-serif"
styles['color-blue'] = '#60cae5'
styles['color-focusable-blue'] = '#91d0d9'
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Commented about this above.

@@ -0,0 +1,20 @@
import validations from './validations'
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👏

Comment thread src/utils/validations.js
@@ -0,0 +1,16 @@
/* eslint-disable no-useless-escape */
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👏

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants