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 items now have unique, stable keys (#1046) #1335

Merged
merged 8 commits into from
Jul 9, 2019
Merged
1 change: 1 addition & 0 deletions docs/advanced-customization.md
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ The following props are part of each element in `items`:
- `hasRemove`: A boolean value stating whether the array item can be removed.
- `hasToolbar`: A boolean value stating whether the array item has a toolbar.
- `index`: A number stating the index the array item occurs in `items`.
- `key`: A stable, unique key for the array item.
- `onDropIndexClick: (index) => (event) => void`: Returns a function that removes the item at `index`.
- `onReorderClick: (index, newIndex) => (event) => void`: Returns a function that swaps the items at `index` with `newIndex`.
- `readonly`: A boolean value stating if the array item is read-only.
Expand Down
18 changes: 18 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 3 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,9 @@
"lodash.pick": "^4.4.0",
"lodash.topath": "^4.5.2",
"prop-types": "^15.5.8",
"react-is": "^16.8.4"
"react-is": "^16.8.4",
"react-lifecycles-compat": "^3.0.4",
"shortid": "^2.2.14"
},
"devDependencies": {
"@babel/cli": "^7.4.4",
Expand Down
5 changes: 4 additions & 1 deletion playground/samples/customArray.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,10 @@ function ArrayFieldTemplate(props) {
<div className={props.className}>
{props.items &&
props.items.map(element => (
<div key={element.index}>
<div
key={element.key}
id={`array-item-${element.key}`}
className={element.className}>
<div>{element.children}</div>
{element.hasMoveDown && (
<button
Expand Down
120 changes: 103 additions & 17 deletions src/components/fields/ArrayField.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import AddButton from "../AddButton";
import IconButton from "../IconButton";
import React, { Component } from "react";
import { polyfill } from "react-lifecycles-compat";
import includes from "core-js/library/fn/array/includes";
import * as types from "../../types";

Expand All @@ -18,6 +19,7 @@ import {
toIdSchema,
getDefaultRegistry,
} from "../../utils";
import shortid from "shortid";

function ArrayFieldTitle({ TitleField, idSchema, title, required }) {
if (!title) {
Expand All @@ -44,7 +46,10 @@ function DefaultArrayItem(props) {
fontWeight: "bold",
};
return (
<div key={props.index} className={props.className}>
<div
key={props.key}
id={`array-item-${props.key}`}
Copy link
Member

Choose a reason for hiding this comment

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

I don't think there's any point in exposing the key to the user as the id? Currently ids are only used as per the idSchema (for example, root_listOfStrings_0) so having another function for id may also not be the best.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What would you recommend here?

Copy link
Member

@epicfaace epicfaace Jul 3, 2019

Choose a reason for hiding this comment

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

Can we just remove the id?

Or is there a particular reason why you thought of adding it?

Copy link
Contributor Author

@fsteger fsteger Jul 8, 2019

Choose a reason for hiding this comment

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

I'm using the id right now to ensure that the key does not change inside the ArrayField tests. If you remove the id, the tests for inserting new rows will fail. The re-order and delete tests only pass when the attribute is removed because the id both before and after the action are undefined, but these should really test to ensure an id exists as well.

If we remove the id, I'm not sure how these tests should be updated to verify the correct behavior as the key doesn't appear to be exposed anywhere else.

Regarding the idSchema, were you thinking of just passing the key into the toIdSchema method for each item, and then accessing it via props.idSchema.key?

Copy link
Member

@epicfaace epicfaace Jul 8, 2019

Choose a reason for hiding this comment

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

Got it. I don't think we should add it to the idSchema or expose it as the id attribute, though, given that the array item ids have no meaning apart from being unique and stable (as opposed to the other ids in the idSchema).

Here are two possible solutions:

  • Can we expose the key as data-rjsf-itemkey or something instead?
  • Or, we don't expose the key at all -- in our tests, create a custom ArrayFieldTemplate that exposes the key in the DOM so that the tests can access it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the id attribute, and updated the tests to use a custom template.

className={props.className}>
<div className={props.hasToolbar ? "col-xs-9" : "col-xs-12"}>
{props.children}
</div>
Expand Down Expand Up @@ -174,6 +179,25 @@ function DefaultNormalArrayFieldTemplate(props) {
);
}

function generateRowId() {
return shortid.generate();
}

function generateKeyedFormData(formData) {
return !Array.isArray(formData)
? []
: formData.map(item => {
return {
key: generateRowId(),
item,
};
});
}

function keyedToPlainFormData(keyedFormData) {
return keyedFormData.map(keyedItem => keyedItem.item);
}

class ArrayField extends Component {
static defaultProps = {
uiSchema: {},
Expand All @@ -185,6 +209,32 @@ class ArrayField extends Component {
autofocus: false,
};

constructor(props) {
super(props);
const { formData } = props;
const keyedFormData = generateKeyedFormData(formData);
this.state = {
keyedFormData,
};
}

static getDerivedStateFromProps(nextProps, prevState) {
const nextFormData = nextProps.formData;
const previousKeyedFormData = prevState.keyedFormData;
const newKeyedFormData =
nextFormData.length === previousKeyedFormData.length
? previousKeyedFormData.map((previousKeyedFormDatum, index) => {
return {
key: previousKeyedFormDatum.key,
item: nextFormData[index],
};
})
: generateKeyedFormData(nextFormData);
epicfaace marked this conversation as resolved.
Show resolved Hide resolved
return {
keyedFormData: newKeyedFormData,
};
}

get itemTitle() {
const { schema } = this.props;
return schema.items.title || schema.items.description || "Item";
Expand Down Expand Up @@ -217,24 +267,40 @@ class ArrayField extends Component {

onAddClick = event => {
event.preventDefault();
const { schema, formData, registry = getDefaultRegistry() } = this.props;
const { schema, registry = getDefaultRegistry(), onChange } = this.props;
const { definitions } = registry;
let itemSchema = schema.items;
if (isFixedItems(schema) && allowAdditionalItems(schema)) {
itemSchema = schema.additionalItems;
}
this.props.onChange([
...formData,
getDefaultFormState(itemSchema, undefined, definitions),
]);
const newFormDataRow = getDefaultFormState(
itemSchema,
undefined,
definitions
);
const newKeyedFormData = [
...this.state.keyedFormData,
{
key: generateRowId(),
item: newFormDataRow,
},
];

this.setState(
{
keyedFormData: newKeyedFormData,
},
() => onChange(keyedToPlainFormData(newKeyedFormData))
);
};

onDropIndexClick = index => {
return event => {
if (event) {
event.preventDefault();
}
const { formData, onChange } = this.props;
const { onChange } = this.props;
const { keyedFormData } = this.state;
// refs #195: revalidate to ensure properly reindexing errors
let newErrorSchema;
if (this.props.errorSchema) {
Expand All @@ -249,7 +315,13 @@ class ArrayField extends Component {
}
}
}
onChange(formData.filter((_, i) => i !== index), newErrorSchema);
const newKeyedFormData = keyedFormData.filter((_, i) => i !== index);
this.setState(
{
keyedFormData: newKeyedFormData,
},
() => onChange(keyedToPlainFormData(newKeyedFormData), newErrorSchema)
);
};
};

Expand All @@ -259,7 +331,7 @@ class ArrayField extends Component {
event.preventDefault();
event.target.blur();
}
const { formData, onChange } = this.props;
const { onChange } = this.props;
let newErrorSchema;
if (this.props.errorSchema) {
newErrorSchema = {};
Expand All @@ -275,18 +347,24 @@ class ArrayField extends Component {
}
}

const { keyedFormData } = this.state;
function reOrderArray() {
// Copy item
let newFormData = formData.slice();
let _newKeyedFormData = keyedFormData.slice();

// Moves item from index to newIndex
newFormData.splice(index, 1);
newFormData.splice(newIndex, 0, formData[index]);
_newKeyedFormData.splice(index, 1);
_newKeyedFormData.splice(newIndex, 0, keyedFormData[index]);

return newFormData;
return _newKeyedFormData;
}

onChange(reOrderArray(), newErrorSchema);
const newKeyedFormData = reOrderArray();
this.setState(
{
keyedFormData: newKeyedFormData,
},
() => onChange(keyedToPlainFormData(newKeyedFormData), newErrorSchema)
);
};
};

Expand Down Expand Up @@ -367,7 +445,8 @@ class ArrayField extends Component {
const itemsSchema = retrieveSchema(schema.items, definitions);
const arrayProps = {
canAdd: this.canAddItem(formData),
items: formData.map((item, index) => {
items: this.state.keyedFormData.map((keyedItem, index) => {
const { key, item } = keyedItem;
const itemSchema = retrieveSchema(schema.items, definitions, item);
const itemErrorSchema = errorSchema ? errorSchema[index] : undefined;
const itemIdPrefix = idSchema.$id + "_" + index;
Expand All @@ -379,6 +458,7 @@ class ArrayField extends Component {
idPrefix
);
return this.renderArrayFieldItem({
key,
index,
canMoveUp: index > 0,
canMoveDown: index < formData.length - 1,
Expand Down Expand Up @@ -544,7 +624,8 @@ class ArrayField extends Component {
disabled,
idSchema,
formData,
items: items.map((item, index) => {
items: this.state.keyedFormData.map((keyedItem, index) => {
const { key, item } = keyedItem;
const additional = index >= itemSchemas.length;
const itemSchema = additional
? retrieveSchema(schema.additionalItems, definitions, item)
Expand All @@ -565,6 +646,7 @@ class ArrayField extends Component {
const itemErrorSchema = errorSchema ? errorSchema[index] : undefined;

return this.renderArrayFieldItem({
key,
index,
canRemove: additional,
canMoveUp: index >= itemSchemas.length + 1,
Expand Down Expand Up @@ -597,6 +679,7 @@ class ArrayField extends Component {

renderArrayFieldItem(props) {
const {
key,
index,
canRemove = true,
canMoveUp = true,
Expand Down Expand Up @@ -658,6 +741,7 @@ class ArrayField extends Component {
hasMoveDown: has.moveDown,
hasRemove: has.remove,
index,
key,
onDropIndexClick: this.onDropIndexClick,
onReorderClick: this.onReorderClick,
readonly,
Expand All @@ -669,4 +753,6 @@ if (process.env.NODE_ENV !== "production") {
ArrayField.propTypes = types.fieldProps;
}

polyfill(ArrayField);

export default ArrayField;
Loading