Skip to content

Commit

Permalink
v0.19.1
Browse files Browse the repository at this point in the history
  • Loading branch information
@aesopwolf
aesopwolf committed Jan 18, 2017
1 parent ac301f8 commit 57f97cb
Show file tree
Hide file tree
Showing 3 changed files with 3 additions and 3 deletions.
2 changes: 1 addition & 1 deletion package.json
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "formsy-react",
"version": "0.19.0",
"version": "0.19.1",
"description": "A form input builder and validator for React JS",
"repository": {
"type": "git",
Expand Down
Loading

18 comments on commit 57f97cb

@gilbarbara
Copy link

Choose a reason for hiding this comment

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

Hey @aesopwolf
What changed in 0.19.1? Any breaking changes? The behavior of all my forms using Formsy has changed..

@aesopwolf
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Shoot, I might have to roll back. I was worried this might happen. Validation logic has changed.

See the code here ac301f8

And the issue it solved here #276

@charleskoehl
Copy link

@charleskoehl charleskoehl commented on 57f97cb Jan 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@aesopwolf, this work-around, modified from the last time I posted it, illustrates what I think is the behavior desired by many. It only displays errors for empty required fields on the onInvalidSubmit event. (using formsy-material-ui 0.5.3 and formsy-react 0.18.1)

class FormContainer extends Component {
  
  submitForm(data) {
    console.log('valid data', data)
  }
  
  notifyFormError(data, resetForm, invalidateForm) {
    var errs = {}
    this.refs.form.inputs.forEach((input) => {
      if (input.showRequired()) {
        errs[input.props.name] = typeof input.props.validationError === 'string' ? input.props.validationError : 'Please fill out this field.'
      }
    })
    invalidateForm(errs)
  }
  
  render() {
    return (
      <Formsy.Form
        onValidSubmit={this.submitForm}
        onInvalidSubmit={this.notifyFormError}
        noValidate
        ref="form"
      >
        // some inputs
      </Formsy.Form>
    )
  }
}

@aesopwolf
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@charleskoehl admittedly it would be great to have it built in to formsy itself though right? Having to write code that detailed/complex to enable standard behavior (or what most would consider should be standard) sounds like a bad developer experience.

@gilbarbara
Copy link

Choose a reason for hiding this comment

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

I find it weird to show errors for empty inputs on load, feels like bad UX.
Since the dev can write custom validations and/or control the error class in the custom input (if required && !value), it should change the behavior if needed instead of the library triggering errors for everything..

@aesopwolf
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@gilbarbara you're right, that's weird behavior for errors with empty inputs to show on load. Would you mind sharing what your custom form element looks like?

@gilbarbara
Copy link

Choose a reason for hiding this comment

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

@aesopwolf It's a huge beast: https://gist.github.com/gilbarbara/89f8af872d812688074a3076118c668b

But the relevant bits are these:

 const hasValue = (getValue() && isValidValue(getValue()) ? 'success' : null);
 const validateClass = showError() ? 'danger' : hasValue;

@aesopwolf
Copy link
Collaborator Author

@aesopwolf aesopwolf commented on 57f97cb Jan 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@gilbarbara thanks for sharing that. You should be able to fix your issue by using isFormSubmitted() before showing your errors.

However, this was my fault. I introduced a breaking change without bumping the major version. I guess the nature of being pre 1.0 is that it's common for breaking changes to happen. But really, since people are using this in production we should already be at 1.0.

I could use some feedback here. I'm not sure if I should:

  1. Roll back the breaking change
  2. Bump the major version (is formsy-react ready to have a 1.0 label?)
  3. Embrace the "beta" nature of pre 1.0 and have everyone update their components to use isFormSubmitted()

@aesopwolf
Copy link
Collaborator Author

@aesopwolf aesopwolf commented on 57f97cb Jan 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Or another halfway option would be to bump the minor version to 0.20.0 to indicate the breaking change. I feel like that's kind of common with pre 1.0 libraries.

@brianfegan
Copy link

Choose a reason for hiding this comment

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

I think the minor version bump likely makes more sense. If you're in production, you should probably be version locking anyway.

But it is also probably time to consider a 1.0 rc.

@aesopwolf
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@brianfegan Good point about version locking in production.

How about we:

  1. Immediately roll back the change in the next 0.19.2 release (and publish asap)
  2. Re-release the breaking change under 0.20.0 (or just wait until 1.0?)
  3. Put together a wish-list of 1.0 features, changes, docs, etc.
  4. Release a 1.0rc in the coming weeks

Does that sound like a good course of action?

@brianfegan
Copy link

Choose a reason for hiding this comment

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

That looks good to me. I think a breaking change in a 0.x update is ok. But I'm locked at 0.18.1 so I'm sort of biased.

@gilbarbara
Copy link

Choose a reason for hiding this comment

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

hey,

yeah, I think is time for a 1.0 release since this library is over 2 years old and it hasn't changed much in the past year. The 0.x releases was an attempt to keep up with the old React versioning but it doesn't make sense anymore.

@aesopwolf
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should we make 1.0 a "big grand" release? Meaning we update all of the docs and examples with 'es6 class' examples (currently we're advertising mixins throughout the docs) and also attempt to close all of the open issues and add open feature requests?

Or just make it easy and only introduce this one breaking change as the new 1.0 feature? (Still have to update the docs to advertise the need for isFormSubmitted() though, but not a big deal).

Some of the breaking changes I see in the future are:

  1. Disregard disabled inputs in the model
  2. Allow for multiple inputs to have the same name and save them to an array - since that's standard HTML form behavior

So I'm just unsure if I want to hammer those into a 1.0 release or wait for 2.0.

@gilbarbara
Copy link

Choose a reason for hiding this comment

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

I'm not a fan of "big grand" releases and I feel that 1.0 should be a "reborn" release, simple and achievable. The docs definitely need to be rewritten with ES6 examples but that's not hard to do and I could help with that.

[s]

@gilbarbara
Copy link

Choose a reason for hiding this comment

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

About isFormSubmitted.. My forms are disabled until all the fields are validated so how would that work?

@aesopwolf
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, good point. Maybe the initial pull request needed more thought before I merged it in.

I'll release 0.19.2 right now that rolls back the change. Then we can do some more exploring. Especially for forms that are disabled until all fields are valid.

@aesopwolf
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just published 0.19.2 to npm. Sorry for all of the trouble.

I've setup a 1.0.0 milestone.

Please sign in to comment.