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

setting default value for the string input refills the field when emptied up #471

Closed
Liooo opened this issue Feb 14, 2017 · 14 comments
Closed

Comments

@Liooo
Copy link

Liooo commented Feb 14, 2017

As is shown below from the playground page.
bug

Is this a desired behaviour? If so, is there any options to set initial value while allowing the field to be empty?

@n1k0
Copy link
Collaborator

n1k0 commented Feb 14, 2017

This looks like a bug indeed!

@n1k0
Copy link
Collaborator

n1k0 commented Feb 14, 2017

This looks to be a side effect of #442; now we propagate undefined when a text field is cleared, the default value defined in the schema is reassigned automatically.

@n1k0
Copy link
Collaborator

n1k0 commented Feb 14, 2017

I'm adding field clearing buttons in #472. That's always been my initial plan, I'm sticking to it.

@Liooo
Copy link
Author

Liooo commented Feb 14, 2017

@n1k0 Thanks for the quick response.

@n1k0
Copy link
Collaborator

n1k0 commented Feb 14, 2017

@Liooo I've deployed #472 to gh-pages, would you mind providing feedback? thanks

@crumblix
Copy link
Collaborator

The problem has simply shifted to "clear". (check out the example on the current live playground).

The real fix to is only apply defaults on create of the item.

At that point you can have your cake and eat it too. You can have a simple UI and have required strings that work as the user expects without "clearing" buttons, or complex ones with the clear button.

While I like the clear button, I do not expect my users will understand it in the context of strings and single checkboxes. Yes they'll get the "clear" part on a string ... but understanding that that is difference from manually deleting the string is something they won't get. And then we hit the same problem as earlier again. So still very much against reverting #442.

Perhaps an option on whether to treat "empty string" as "undefined" once we have the clear boxes? It can default either way, but I personally don't want the UI acting differently if the users hit the "clear" or manually deletes the string.

@crumblix
Copy link
Collaborator

crumblix commented Feb 15, 2017

OK ... I've investigated this further. It actually looks to me like it's a bug in StringField.js.

On line 39. Simply replace:

    value={defaultFieldValue(formData, schema)}

with

    value={formData}

This way the default only applies on creation. This should work independently I think on whether you clear it manually or use the clear button.

Currently in the live environment if you clear it displays "bazinga" on the screen but it's null in the formData. I would expect this change would work with both that and #442.

Note there is also what appears to be a bug with the current implementation regardless of this change (and still exists afterwards). The defaults in general appear on screen but not in the formData. I tested this on the simple tab of the playground by making the following changes (necessarily in code):

firstName: {
  type: "string",
  title: "First name",
  default: "Chuck"
}

formData: {
  lastName: "Norris",
  age: 75,
  bio: "Roundhouse kicking asses since 1940",
  password: "noneed"
}

"Chuck" will appear on the form ... but not in the formData. If I were to hit "Submit" now as a user I would expect "Chuck" to be submitted but it wouldn't.

As far as I can see it works as well if not better than the current situation, which to me appears there are other default issues.

Unless there is some use-case I'm missing or not aware of or something I'm missing?

@crumblix
Copy link
Collaborator

Mind you if you make any OTHER change the default string will propagate to the formData ... so it would only be noticeable on a form submitted with only defaults. Uncommon but certainly possible.

@Liooo
Copy link
Author

Liooo commented Feb 15, 2017

@n1k0 Thanks for that.
Think, for most of users it'd be more natural being able to just clear all characters without clear button, though clear button is a good idea as itself apart from this.

As crumblix says, if it's a bug and fixable, that'd be the best.
#471 (comment)

@n1k0
Copy link
Collaborator

n1k0 commented Feb 16, 2017

Ok, the clear buttons approach is certainly overkill here, and probably too ambitious. I've implemented @crumblix's suggested fix in #476.

@knilink
Copy link
Contributor

knilink commented Feb 20, 2017

@n1k0 Just wondering if it is better to let users handling the default value themselves rather than do it inside the form engine? Such as

// initialize form
const formData = Form.getDefaultFromData(jsonSchema, defaultFormData);

I believed handling default value brough the "setState" in ObjectField and ArrayField which was undesirable and also caused the issue #339.

@n1k0
Copy link
Collaborator

n1k0 commented Feb 20, 2017

@knilink I remember seeing your comment in #339 but I'm still struggling figuring out all the implications. Especially as I never managed to reproduce the initial perf pb with lost characters myself... But I feel like you may be on something here. Feel free to open a PR about it.

@n1k0
Copy link
Collaborator

n1k0 commented Feb 20, 2017

Meanwhile I think #476 fixes this very issue and even if transitional to something better, is probably something worth considering on the short term. Feedback would be highly appreciated.

@n1k0 n1k0 closed this as completed in 72e40b7 Feb 22, 2017
n1k0 added a commit that referenced this issue Feb 22, 2017
Highlights
---

- Improved performance and reactivity.
- More consistent validation behavior and UX for array field items.

Backward incompatible changes
---

- `ObjectField` and `ArrayField` are now stateless components, their
  local state handling has been dropped entirely, resulting in large
  performance improvements.
- The `defaultFieldValue` helper from the `utils` module has been
  removed, as it wasn't used anymore.

New features
---

* Fix #411:  Enable required field on radio options. (#469)
* Spread `formContext` to `ArrayTemplateField`, `files` and `multiselect`
  arrays (#456)
* From #471: Non-nullable array item fields are now marked as required in
  the UI.

Bugfixes
---

* Don't pass consumed `SchemaField` class names to child component (#439)
* Turn `ObjectField` and `ArrayField` into stateless components (#480)
* Fix #471: Drop default value initialization at the widget level. (#476)

Kudos
---

Special thanks to @crumblix and @knilink for their help on this release.
You guys rock!
@n1k0
Copy link
Collaborator

n1k0 commented Feb 22, 2017

Released in v0.43.0.

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

No branches or pull requests

4 participants