Skip to content

ArrayFieldTemplate#437

Merged
n1k0 merged 22 commits intorjsf-team:masterfrom
dehli:develop
Jan 17, 2017
Merged

ArrayFieldTemplate#437
n1k0 merged 22 commits intorjsf-team:masterfrom
dehli:develop

Conversation

@dehli
Copy link
Copy Markdown
Contributor

@dehli dehli commented Jan 4, 2017

Reasons for making this change

Hello again! I've decided to create a new PR since I changed how the custom arrays were implemented based on feedback on my last PR (#395). Sorry it took a while!

With this change, you can customize how your arrays are rendered by passing an ArrayFieldTemplate object, very similar to the FieldTemplate.

I'd love some feedback! My initial thoughts are that I'd rather pass removeItem, moveItemDown, and moveItemUp to each item in the array (so that you don't need to pass the index to the functions). The root array could then store the existing functions that take indices.

This PR is related to #354 and will make the library much more flexible.

Here's the playground so you can see what it looks like!

Thanks!

Edit: I'll get that test passing. Forgot to run that!

Checklist

  • I'm updating documentation
    • I've checked the rendering of the Markdown text I've added
    • If I'm adding a new section, I've updated the Table of Content
  • I'm adding or updating code
    • I've added and/or updated tests
    • I've updated docs if needed
  • I'm adding a new feature
    • I've updated the playground with an example use of the feature

@olzraiti
Copy link
Copy Markdown
Contributor

Great work! Thanks for the rewrite, this looks really good. 👍

@dehli
Copy link
Copy Markdown
Contributor Author

dehli commented Jan 12, 2017

Thanks @olzraiti! 😄

Copy link
Copy Markdown
Collaborator

@n1k0 n1k0 left a comment

Choose a reason for hiding this comment

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

This looks great, thank you!

Would you mind adding a few tests covering the custom array field template case?

README.md Outdated
- [Form attributes](#form-attributes)
- [Advanced customization](#advanced-customization)
- [Field template](#field-template)
- [Array template](#array-template)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nit: Array field template maybe

README.md Outdated
- `index`: number
- `onDropIndexClick`: (index) => void
- `onReorderClick`: (index, newIndex) => void
- `readonly`: boolean
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

General remark for these two lists: it would be nice to have English sentences describing the purpose of each property, for consistency with the rest of the docs :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good idea! I'll improve the docs I wrote a bit!

@@ -0,0 +1,38 @@
import React, { Component } from "react";

const ArrayFieldTemplate = props => (
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please use a regular function declaration here, for consistency with the rest of the code in this project.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sounds good 👍

};
Single: single,
"Custom Array": customArray
}; No newline at end of file
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nit: all files need an empty line at EOF.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

👍

<div className="field-description" key={`field-description-${props.idSchema.$id}`}>
{props.schema.description}
</div>
) : null}
Copy link
Copy Markdown
Collaborator

@n1k0 n1k0 Jan 17, 2017

Choose a reason for hiding this comment

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

Nit: We're using either the foo && <Bar/> or foo ? <Bar/> : null checks, we should decide for one single style and use it consistently across the project. I don't have a strong opinion on which one is better, though I like the concision of the && version.

Note that if we go with the latter, this will have to be submitted in a followup PR to avoid cluttering this one, which is large enough already.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I also prefer using && for brevity. I'll hold off on this change though, like you said.


<div className="row array-item-list"
key={`array-item-list-${props.idSchema.$id}`}>
{props.items && props.items.map(p => DefaultArrayItem(p))}
Copy link
Copy Markdown
Collaborator

@n1k0 n1k0 Jan 17, 2017

Choose a reason for hiding this comment

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

Nit: I'm curious as to if props.items.map(DefaultArrayItem) would work here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch! That should work. I'll test it out, and update accordingly.

if (event) {
event.preventDefault();
event.target.blur();
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm curious as to why this is needed? Test env?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I added this so that you could programmatically call the onDropIndexClick()() without a MouseEvent argument.

@dehli
Copy link
Copy Markdown
Contributor Author

dehli commented Jan 17, 2017

Thanks for the comments! I'll get those changes in, plus those tests :)

@dehli
Copy link
Copy Markdown
Contributor Author

dehli commented Jan 17, 2017

Hey @n1k0, I just addressed the comments you made. Let me know if there's anything else you'd like changed! Thanks!

Copy link
Copy Markdown
Collaborator

@n1k0 n1k0 left a comment

Choose a reason for hiding this comment

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

A few nits in tests and we're good to go.

items: {type: "string"}
};

const {node} = createFormComponent({
Copy link
Copy Markdown
Collaborator

@n1k0 n1k0 Jan 17, 2017

Choose a reason for hiding this comment

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

Please move this to a beforeEach statement, so the element is not created as soon as the describe call is executed. Created troubles in the past. Please refer to existing tests to see how it's done.

ArrayFieldTemplate,
formData,
schema
});
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ditto.

Copy link
Copy Markdown
Collaborator

@n1k0 n1k0 left a comment

Choose a reason for hiding this comment

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

👍

@dehli
Copy link
Copy Markdown
Contributor Author

dehli commented Jan 17, 2017

Appreciate the feedback @n1k0 👍

@n1k0 n1k0 merged commit 97f30d3 into rjsf-team:master Jan 17, 2017
@n1k0
Copy link
Copy Markdown
Collaborator

n1k0 commented Jan 17, 2017

This is a very nice feature, thanks for the great work on this.

@dehli
Copy link
Copy Markdown
Contributor Author

dehli commented Jan 17, 2017

Of course! Thanks to you and your team for a great library!

n1k0 added a commit that referenced this pull request Jan 28, 2017
Breaking changes

When a text input is emptied by the user, it's value is now reset to `undefined` instead of being set to `""` (empty string) as previously. This better matches traditional HTML forms behavior.

New features

* Add an array field template component (#437)
* Wrap radio and checkbox labels into span to enable styling. (#428)
* Reset text inputs value when emptied (#442)
* Add transform hook for changing errors from json schema validation (#432)
* Add a noHtml5Validate prop (#448)
* Add support for a onBlur event handler (#431)
* Allow empty option for enum fields (#451)

Bugfixes

* Fix #452: Support recursively referenced definitions. (#453)
* Fix #454: Document what master actually is, suffix its version with -dev.
@n1k0
Copy link
Copy Markdown
Collaborator

n1k0 commented Jan 28, 2017

Released in v0.42.0.

@GoodVaibz
Copy link
Copy Markdown

@dehli Any chance you can rehost the playground. I am not 100% sure this is the functionality I am looking for

@dehli
Copy link
Copy Markdown
Contributor Author

dehli commented Jul 27, 2017

Hey @Vaibhav430, the official playground contains what I had hosted. Just click on Custom Array!

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.

4 participants