Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Remove status from Form state (BUG FIX) #660

Merged
merged 2 commits into from
Aug 16, 2017

Conversation

olzraiti
Copy link
Contributor

@olzraiti olzraiti commented Aug 9, 2017

Reasons for making this change

This fixes a bug that causes the error list to flicker when formData is edited.

To reproduce: go to the "Errors" tab on the playground, select an input on the Form and start smashing the keyboard (you don't really even have to smash the keyboard, you can see the effect on any single form data change). The error list will flicker.

First I thought the bug was caused by some recent commits regarding the Error List, but actually the flickering is caused by the status state property, which will flip between initial and editing during typing.

The status property has been there from the very first commit, but I can't find any real use for it other than causing this bug. I don't understand why we wouldn't show the error list when the Form is edited. Even if there was some kind of buffering that would hide the error list only when the formData is updated in intervals less time than some milliseconds, it would affect the page scrolling position before and after the user stops typing. The only side effect that removing status causes that I can think of is that when onSubmit calls this.props.onSubmit(this.state), the state won't contain status: "submitted" - which seems kind of pointless to me :)

Checklist

  • I'm updating documentation
    • I've checked the rendering of the Markdown text I've added
    • If I'm adding a new section, I've updated the Table of Content
  • I'm adding or updating code
    • I've added and/or updated tests
    • I've updated docs if needed
    • I've run npm run cs-format on my branch to conform my code to prettier coding style
  • I'm adding a new feature
    • I've updated the playground with an example use of the feature

@n1k0
Copy link
Collaborator

n1k0 commented Aug 16, 2017

The initial purpose for having a status was to distinguish between a pristine form and one that has been edited already. I agree this was never really used by our team, so I'm not against removing it entirely, though we should ensure mentioning it in the next release changelog because it's a breaking change of the public API.

@olzraiti
Copy link
Contributor Author

Hmm, okay. We could keep that functionality by adding status: "submitted" to every onSubmit propagation.

So we would change this part:

 if (this.props.onSubmit) {
  this.props.onSubmit(this.state);
}

To this:

 if (this.props.onSubmit) {
  this.props.onSubmit({...this.state, status: "submitted"});
}

This way, we wouldn't have any breaking changes to the public API.

Either that, or just remove the functionality. Which would you prefer? :)

@n1k0
Copy link
Collaborator

n1k0 commented Aug 16, 2017

Yeah, keeping BC while fixing bugs always sounds like a great plan to me

@n1k0 n1k0 merged commit 44e1f21 into rjsf-team:master Aug 16, 2017
@cdtinguria
Copy link

Guys, when will this fix be released?

@n1k0
Copy link
Collaborator

n1k0 commented Aug 20, 2017

Probably next week

@glasserc
Copy link
Contributor

glasserc commented Sep 6, 2017

Released in v0.50.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants