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

Array field optional sortability #345

Merged
merged 32 commits into from
Oct 20, 2016
Merged
Show file tree
Hide file tree
Changes from 27 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
251974c
fix HiddenWidget error when value is undefined
olzraiti Jul 12, 2016
8e69465
add context prop
olzraiti Jul 20, 2016
7ae018c
Replace tabs with spaces and add context PropType to Form.
olzraiti Jul 20, 2016
1217215
Pass context to FieldTemplate
olzraiti Aug 19, 2016
4136956
Fix passing context to SchemaField
olzraiti Aug 19, 2016
f8158b9
Replace tabs with spaces
olzraiti Aug 19, 2016
9eeaf75
replace context with formContext
olzraiti Aug 26, 2016
39fb8c3
Don't pass formContext to fields that don't use it
olzraiti Aug 26, 2016
7ada187
Remove duplicate default export from Form
olzraiti Aug 26, 2016
6ab8b12
Add tests for formContext
olzraiti Aug 26, 2016
933f808
Fix formContext tests lint issues
olzraiti Aug 26, 2016
b7670f4
FormContext is passed internally inside registry. Pass formContext to…
olzraiti Aug 29, 2016
770f761
update README
olzraiti Aug 29, 2016
295e572
fix lint and package.json
olzraiti Aug 29, 2016
7bc5952
fix tests. FormContext must be and object.
olzraiti Aug 29, 2016
90c0bd8
Fix minor style issues
olzraiti Aug 30, 2016
2861c92
Update README
olzraiti Aug 30, 2016
9e0caac
Merge branch 'master' of https://github.com/mozilla-services/react-js…
olzraiti Aug 30, 2016
9c158ed
Use empty title instead of name
olzraiti Aug 30, 2016
4974362
Use empty label instead of name
olzraiti Aug 30, 2016
8d120e4
Add tests for passing titles/labels
olzraiti Sep 30, 2016
a919ae1
fix lint issues
olzraiti Sep 30, 2016
e936e07
ArrayField sorting can be disabled
olzraiti Oct 18, 2016
0d2f33f
replace tabs with spaces
olzraiti Oct 18, 2016
0283a95
update README
olzraiti Oct 18, 2016
4c68350
Merge branch 'master' of https://github.com/mozilla-services/react-js…
olzraiti Oct 18, 2016
b7465cf
Remove empty line
olzraiti Oct 18, 2016
32c77e7
Rename ArrayField options 'sortable' to 'orderable' & minor changes
olzraiti Oct 19, 2016
a7e917e
fix lint issues
olzraiti Oct 19, 2016
f16c111
Some code style improvement
olzraiti Oct 19, 2016
b8adc95
replace tabs with spaces
olzraiti Oct 19, 2016
b34235f
Update README
olzraiti Oct 20, 2016
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
19 changes: 19 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ A [live playground](https://mozilla-services.github.io/react-jsonschema-form/) i
- [File widgets](#file-widgets)
- [Multiple files](#multiple-files)
- [Object fields ordering](#object-fields-ordering)
- [Array items ordering](#array-items-ordering)
- [Custom CSS class names](#custom-css-class-names)
- [Custom labels for enum fields](#custom-labels-for-enum-fields)
- [Multiple choices list](#multiple-choices-list)
Expand Down Expand Up @@ -399,6 +400,24 @@ render((
uiSchema={uiSchema} />
), document.getElementById("app"));
```
### Array items ordering

Array items are sortable by default. The `uiSchema` object spec allows you to disable ordering:
Copy link
Contributor

Choose a reason for hiding this comment

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

are orderable by default, and react-jsonschema-form renders move up/down buttons along them.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would probably say "alongside" :)


```jsx
const schema = {
type: "array",
properties: {
type: "string"
}
};

const uiSchema = {
"ui:options": {
sortable: false
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that @n1k0 took a different approach in #346 in order to introduce widget options:

const uiSchema = {
  "ui:widget": {
    component: "checkboxes",
    options: {
      inline: true
    }
  }
};

I think that both approaches are valid :) I'll let you figure out pros and cons for each ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Having the same approach as widgets would be more consistent, but the ui:options -style makes updating a uiSchema simpler (if you want to add an option to an existing uiSchema field, you don't replace any lines, just add new ones).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I actually like the ui:options approach, definitely less verbose and more flexible. Too bad I've just landed #346 which leverages the widget options approach, which is making your approach inconsistent.

Don't rush reverting though, I may actually just switch to using your ui:options thing instead :)

};
```

### Custom CSS class names

Expand Down
9 changes: 8 additions & 1 deletion src/components/fields/ArrayField.js
Original file line number Diff line number Diff line change
Expand Up @@ -337,8 +337,15 @@ class ArrayField extends Component {
autofocus
}) {
const {SchemaField} = this.props.registry.fields;
const {disabled, readonly} = this.props;
const {disabled, readonly, uiSchema} = this.props;

const sortable = uiSchema["ui:options"] ? uiSchema["ui:options"].sortable : true;
Copy link
Contributor

Choose a reason for hiding this comment

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

What If options is defined but sortable is left undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got me there! Fixed. I added also a PropType for safety. Now the only weird thing that could happen would be when orderable is set specifically to undefined:

...
"ui:options": {
  orderable: undefined
}
...

The PropType won't catch undefined, so we interpret it as false. It's unclear what the user would mean with undefined - I think false is a safe bet since it's a falsy value.

if (!sortable) {
canMoveUp = false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

As a general good practice, I'd rather not mutate passed arguments at all.

canMoveDown = false;
}
const hasToolbar = removable || canMoveUp || canMoveDown;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually the same could be achieved by this (I think):

const {orderable} = {orderable: true, ...uiSchema["ui:options"]};
const hasToolbar = orderable && (removable || canMoveUp || canMoveDown);


const btnStyle = {flex: 1, paddingLeft: 6, paddingRight: 6, fontWeight: "bold"};

return (
Expand Down
9 changes: 9 additions & 0 deletions test/ArrayField_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,15 @@ describe("ArrayField", () => {
expect(moveDownBtns[1].disabled).eql(true);
});

it("should not be sortable when uiSchema[\"ui:options\"].sortable === false", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

should not show move up/down buttons is sortable is false

const {node} = createFormComponent({schema, formData: ["foo", "bar"], uiSchema: {"ui:options": {sortable: false}}});
const moveUpBtns = node.querySelector(".array-item-move-up");
const moveDownBtns = node.querySelector(".array-item-move-down");

expect(moveUpBtns).to.be.null;
expect(moveDownBtns).to.be.null;
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

});

it("should remove a field from the list", () => {
const {node} = createFormComponent({schema, formData: ["foo", "bar"]});
const dropBtns = node.querySelectorAll(".array-item-remove");
Expand Down