Skip to content

removing content from text field result in empty string instead of undefined#539

Merged
n1k0 merged 3 commits intorjsf-team:masterfrom
quentin-sommer:master
Apr 7, 2017
Merged

removing content from text field result in empty string instead of undefined#539
n1k0 merged 3 commits intorjsf-team:masterfrom
quentin-sommer:master

Conversation

@quentin-sommer
Copy link
Copy Markdown
Contributor

@quentin-sommer quentin-sommer commented Apr 6, 2017

Reasons for making this change

As mentioned here before #476 (comment)
The new way of handling empty text fields (setting them to undefined) is undesirable as most of the time we want to keep an empty value.
If we want to delete a field we can do so after the form validation by for emptiness and delete accordingly.

Behavior change:

  • sets text field to "" instead of undefined when you remove everything inside it

Checklist

  • I'm updating documentation
    • I've checked the rendering of the Markdown text I've added
  • 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

Please take in consideration that I'm fairly new to the repo and might have underestimated the change, I'd like your input if that's the case!

Quentin

@n1k0
Copy link
Copy Markdown
Collaborator

n1k0 commented Apr 6, 2017

I'm not sure we want to revert the old behavior. We spent weeks (months) aligning on this and it was hard to reach consensus. Please ensure reading comments in related issues, there are a lot of constraints which may not be obvious at first glance.

Could you please describe a little more your specific use case so we can ensure we're not missing any piece of valuable information here?

Calling @crumblix @olzraiti @revolunet @ @Reggino @knilink @vinhlh @mplis-jetsetter @glasserc @MoOx and other people interested in this project to provide feedback.

@quentin-sommer
Copy link
Copy Markdown
Contributor Author

quentin-sommer commented Apr 6, 2017

Hi,

When implementing data edit forms and dealing with strings I think we have 2 use cases

  • string is used to hold a link information (such as an id to another resource or an enum value).
    In this case, it's easier to mark the emptiness of the field as null or undefined as it's logical that it's either here or non-existant.

  • string is used to hold an arbitrary information (such as a description or a text). In this case, it is better to hold a sane default value for the field (it could be "" or something else depending on the context).

One way to make is this is to leverage the default value and populate the field with it every time it becomes empty. If we want to represent it by undefined we just have to leave default blank. However, if we want to keep a default value at all times we provide it to default and we'll be assured that we will still be getting a good default in any case.

The behavior I'd like to see is the following:

  • when no default field is set and field is empty, output undefined
  • when a default field is set and field is empty, output default value

@mikeplis
Copy link
Copy Markdown

mikeplis commented Apr 6, 2017

I agree that there are two different, valid use cases for an empty string field. I've come across both since I started using this library. I like giving the user some control to define the behavior.

default does feel like a natural place for this, though I worry that that would be overloading the field. We'd be making the assumption that the value you want to populate the field with before the user interacts with it (the current use of default) is always the same value you want in the field when the user clears it, which I don't think is a safe assumption.

@Reggino
Copy link
Copy Markdown
Contributor

Reggino commented Apr 6, 2017

I agree with @quentin-sommer the current string-array handling feels weird and needs some attention :-)

I agree with @MPLIS it would simply be annoying whenever you would try to clear an input and it is instantly reset to some default value.

Can't we just add a new option to the uischema? Something like ui:allowEmptyString? (or ui:initEmptyString, or something more verbose?)

@whollacsek
Copy link
Copy Markdown

I also agree with @MPLIS, how about something like ui:emptyValue, which when not set means undefined, so the developer can explicitly set it to "" (empty string) or 0 (zero). This should keep the current behavior as is, and won't break anything from the issues @n1k0 is referring to.

@quentin-sommer
Copy link
Copy Markdown
Contributor Author

I've implemented the change on the pull request so you can try it out here
https://quentin-sommer.github.io/react-jsonschema-form

the firstName field in the simple example as an ui:emptyValue of ""
and the listOfStrings field of the array example as an ui:emptyValue of ""

@n1k0
Copy link
Copy Markdown
Collaborator

n1k0 commented Apr 7, 2017

This is really good, and it solves all the constraints I had in mind. Could you please add some documentation in the README so we can land this patch? Edit: you've added docs I've missed for some reason. I'm landing this!

@n1k0 n1k0 merged commit 0ef7dfe into rjsf-team:master Apr 7, 2017
n1k0 added a commit that referenced this pull request Apr 7, 2017
New features
============

- The new `ui:emptyValue` uiSchema directive allows having a
  field set to a default value when emptied. (#539)
- Pass uiSchema to custom array templates. (#537)
- Add sharing capability to the Playground. (#535)
@n1k0
Copy link
Copy Markdown
Collaborator

n1k0 commented Apr 7, 2017

Released in v0.46.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.

5 participants