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.

Sign up for GitHub

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

Jump to bottom

Enable Arrays to be marked as required, and fixes for strings marked as required #442

Merged
merged 15 commits into from
Jan 20, 2017
Merged

Enable Arrays to be marked as required, and fixes for strings marked as required #442

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
c30f5b4
Enable the required field on arrays
Nov 24, 2016
cc91fa2
Update documentation
Nov 24, 2016
8ecafc2
Pass empty string text input in as undefined.
Nov 24, 2016
5091525
Typos in documentation.
Jan 18, 2017
8e2ecf1
Make code cleaner
Jan 18, 2017
e0fe72e
Make code cleaner
Jan 18, 2017
b826fc8
Make code cleaner
Jan 18, 2017
2e3c712
Make code cleaner
Jan 18, 2017
477d7de
Make code cleaner
Jan 18, 2017
ea934ed
Revert documentation for marking arrays are requried.
Jan 19, 2017
fe28e93
Change var to const
Jan 19, 2017
5f15775
Revert mandatory changes to arrays.
Jan 20, 2017
7e7a925
Update documentation to make it clear how to achieve the same result …
Jan 20, 2017
4026366
Fix documentation to (hopefully) make clear the distinction between r…
Jan 20, 2017
0e05f7b
Tests for converting empty string to undefined on Text and TextareaWi…
Jan 20, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -558,6 +558,8 @@ const uiSchema = {
};
```

In addition it is valid to mark an array as "required". This means the user must select at least one item to add to the array.
Copy link
Collaborator

@n1k0 n1k0 Jan 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I'm realizing slightly late that this is overlapping with the minItems jsonschema directive. I think the jsonschema spec wants a required object array property to be an array, possibly an empty one. With jsonschema, if we wanted at least one item in our array, we'd set minItems to 1 instead of relying on the required directive.

So that's a strong direction we're taking here, we're officially diverging from the jsonschema spec to fit Web forms use cases & habits. Is everybody okay with this?

Would appreciate inputs from @MoOx @maartenth @olzraiti @leplatrem @Natim @dehli

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fair call. I wasn't aware of that directive. If we can achieve a similar effect with existing code that is standards compliant I would certainly support that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed that's a serious diff. Not sure I am comfortable with that but I guess we can't have the silver bullet.

Copy link
Collaborator Author

@crumblix crumblix Jan 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

The SelectWidget is an interesting case. In truth I don't use it much because I've written my own custom widget based on react-select which better suited my needs for a nullable and multiselect-able drop down widget. (Happy to share if anyone wants it, but I thought since it was an extra dependency it was better off outside the core project.)

I could be convinced to do it here as well, since of course the "required" tag won't work, and while I prefer react-select for this case, perhaps like dates we possibly should cater for the most general case.

Copy link
Contributor

Choose a reason for hiding this comment

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

This wouldn't affect my code base, but I'm not sure if we would benefit enough from adding this considering the consequences.

We aren't extending the json schema spec in a way that would make something possible that was impossible before (using minItem provides the functionality that is wanted here). We are merely making something more convenient than before, with the cost of breaking compatibility with json schema spec.

Copy link
Collaborator

Choose a reason for hiding this comment

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

JSON Schema is not a form builder

The issue is that people import existing schemas and expect them to work with forms with no need to tweak their schemas (uiSchemas have been introduced for that specific purpose).

That could be addressed with an appropriate FAQ/Caveats section though. The same way we could document the minItems jsonschema directive.

the difference between an empty array and undefined is simply not visible in a form

That's indeed a fair point. Another approach would be to make the difference actually visible, but that's rather tricky to get right, and would probably clutter UX a lot. Also this would have to work with bootstrap and other styles.

So my current thinking is:

  1. Keep the empty string as undefined value feature from this patch.
  2. Remove the required array should contain at least one item one and add proper docs describing how to achieve it using the minItems jsonschema directive.
  3. Leave SelectWidget as it is, users should provide enum values matching their expectations.

What do people think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with you @n1k0 . I'm currently using minItems in my application b/c that's the json-schema way of doing it. If we added documentation describing it, then it'll be more obvious that it's the way to do it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good to me! Empty string as undefined seems well-grounded.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm OK with your proposed solution, @n1k0. I think it is an improvement to the current situation either way.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good, @crumblix would you mind updating the patch accordingly if you agree with this decision? :)

Thanks a bunch 👍


See the "Arrays" section of the playground for cool demos.

### Autogenerated widget ids
Expand Down
9 changes: 7 additions & 2 deletions src/components/fields/ArrayField.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,8 +216,12 @@ class ArrayField extends Component {
if (event) {
event.preventDefault();
}
var newitems = this.state.items.filter((_, i) => i !== index);
if (newitems.length == 0) {
newitems = undefined;
}
this.asyncSetState({
items: this.state.items.filter((_, i) => i !== index)
items: newitems
}, {validate: true}); // refs #195
Copy link
Collaborator

Choose a reason for hiding this comment

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

const items = this.state.items.filter((_, i) => i !== index);
this.asyncSetState({
  items: items.length > 0 ? items : undefined,
}, {validate: true}); // refs #195

};
};
Expand Down Expand Up @@ -327,7 +331,7 @@ class ArrayField extends Component {
}

renderMultiSelect() {
const {schema, idSchema, uiSchema, disabled, readonly, autofocus} = this.props;
const {schema, idSchema, uiSchema, disabled, required, readonly, autofocus} = this.props;
const {items} = this.state;
const {widgets, definitions} = this.props.registry;
const itemsSchema = retrieveSchema(schema.items, definitions);
Expand All @@ -338,6 +342,7 @@ class ArrayField extends Component {
<Widget
id={idSchema && idSchema.$id}
multiple
required={required}
onChange={this.onSelectChange}
options={options}
schema={schema}
Expand Down
8 changes: 6 additions & 2 deletions src/components/widgets/BaseInput.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,21 +8,25 @@ function BaseInput(props) {
value,
readonly,
autofocus,
onChange,
options, // eslint-disable-line
schema, // eslint-disable-line
formContext, // eslint-disable-line
registry, // eslint-disable-line
...inputProps
} = props;
const _onChange = props.onChange;
return (
<input
{...inputProps}
className="form-control"
readOnly={readonly}
autoFocus={autofocus}
value={typeof value === "undefined" ? "" : value}
onChange={(event) => onChange(event.target.value)}/>
onChange={(event) => {if (event.target.value == "") {
return _onChange(undefined);
} else {
return _onChange(event.target.value);
}}}/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

const _onChange = ({target: {value}}) => {
  return props.onChange(value === "" ? undefined : value);
};
// ...
onChange={_onChange} />

);
}

Expand Down
9 changes: 7 additions & 2 deletions src/components/widgets/CheckboxesWidget.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,12 @@ function selectValue(value, selected, all) {
}

function deselectValue(value, selected) {
return selected.filter(v => v !== value);
var ret = selected.filter(v => v !== value);
if (ret.length == 0) {
return undefined;
} else {
return ret;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

const ret = selected.filter(v => v !== value);
return ret.length === 0 ? undefined : ret;

}

function CheckboxesWidget(props) {
Expand All @@ -19,7 +24,7 @@ function CheckboxesWidget(props) {
return (
<div className="checkboxes" id={id}>{
enumOptions.map((option, index) => {
const checked = value.indexOf(option.value) !== -1;
const checked = (value === undefined) ? false : value.indexOf(option.value) !== -1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Unnecessary wrapping parenthesis.

const disabledCls = disabled ? "disabled" : "";
const checkbox = (
<span>
Expand Down
7 changes: 6 additions & 1 deletion src/components/widgets/TextareaWidget.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ function TextareaWidget({
autofocus,
onChange
}) {
const _onChange = onChange;
return (
<textarea
id={id}
Expand All @@ -22,7 +23,11 @@ function TextareaWidget({
disabled={disabled}
readOnly={readonly}
autoFocus={autofocus}
onChange={(event) => onChange(event.target.value)}/>
onChange={(event) => {if (event.target.value == "") {
return _onChange(undefined);
} else {
return _onChange(event.target.value);
}}}/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

See previous impl. suggestion above.

);
}

Expand Down