Add ObjectFieldTemplate by olzraiti · Pull Request #653 · rjsf-team/react-jsonschema-form

olzraiti · 2017-08-01T13:02:57Z

Reasons for making this change

ObjectFieldTemplate simplifies customizing how the fields are rendered, eg. creating a bootstrap grid layout or a flexbox layout is easier. Previously we had to override ObjectField to achieve custom layouts - like FieldTemplate and ArrayFieldTemplate, we let the users worry only about rendering. Admittedly, there isn't that much logic behind ObjectField, but this pull requests adds also consistency and makes it possible to customize titles & descriptions without using TitleField and DescriptionField (TitleField and DescriptionField could be even deprecated at some point in order to make the code base more consistent - discussed in #354 also).

EDIT: These issues/pull requests would be solved by this: #623 #268

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've run npm run cs-format on my branch to conform my code to prettier coding style
I'm adding a new feature
- I've updated the playground with an example use of the feature

…React elements

glasserc

Hi, thanks for the PR, plus your other PRs and comments, plus also your patience!

I've started to warm up to the idea of this patch. It does improve our consistency for customizing formatting and it does address some concerns raised in other issues. My main remaining concern is about whether this goes far enough, and whether the approach it takes can be applied to arrays as well as objects. But I don't see any reason why arrays would be much different, and I'm comfortable merging this and leaving arrays for later.

Remaining are basically nits.

glasserc · 2017-09-15T15:34:58Z

README.md

+The following props are passed to each `ObjectFieldTemplate`:
+
+- `DescriptionField`: The generated `DescriptionField` (if you wanted to utilize it)
+- `TitleField`: The generated `TitleField` (if you wanted to utilize it).


I'm kind of ambivalent about passing these field components to the template. Client code already has the opportunity to override the entire template, using any DescriptionField and TitleField definition they want, and they can re-import DescriptionField and TitleField themselves if they want them. So what's the value?

I don't think generated is the right word here -- it's the one taken from the registry.

How about changing if you wanted to to in case you wanted to?

I agree that passing DescriptionField and TitleField is useless. However, ArrayField also passes them to ArrayFieldTemplate, so at least we are consistenly useless :)

I copy pasted the descriptions from ArrayFieldTemplate manual entry. I changed the descriptions in both manual entries now to "The DescriptionField/TitleField from the registry (in case you wanted to utilize it)"

glasserc · 2017-09-15T15:35:31Z

README.md

+- `formData`: The form data for the object.
+- `formContext`: The `formContext` object that you passed to Form.
+
+The following props are part of each element in `items`:


Is items here a typo for properties?

glasserc · 2017-09-15T15:39:56Z

README.md

+- `name`: A string representing the property name.
+- `disabled`: A boolean value stating if the object property is disabled.
+- `index`: A number stating the index the property occurs in the `properties`.
+- `readonly`: A boolean value stating if the property is readonly.


Missing blank line.

glasserc · 2017-09-15T15:40:22Z

README.md

+- `children`: The html for the property's content.
+- `name`: A string representing the property name.
+- `disabled`: A boolean value stating if the object property is disabled.
+- `index`: A number stating the index the property occurs in the `properties`.


I don't think we need to expose this, because it should be the same as the index in the children array, right?

glasserc · 2017-09-15T15:47:02Z

test/ObjectFieldTemplate_test.js

+          <TitleField title={title} />
+          <DescriptionField description={description} />
+          <div>
+            {properties.map(({ children, idx }) => (


Is idx even going to be valid here?

glasserc · 2017-09-15T15:53:05Z

README.md

+
+The following props are part of each element in `items`:
+
+- `children`: The html for the property's content.


I don't like children as the name for this property. How about content, or rendered?

olzraiti · 2017-09-25T07:31:39Z

My main remaining concern is about whether this goes far enough, and whether the approach it takes can be applied to arrays as well as objects. But I don't see any reason why arrays would be much different, and I'm comfortable merging this and leaving arrays for later.

Hmm, I'm not sure I understand what you mean. The templating approach is taken directly from how ArrayField handles ArrayFieldTemplate and the implementation is very similar.

Hi, thanks for the PR, plus your other PRs and comments, plus also your patience!

No problem, there's no hurry :). I'm using a patched private repo so I can use the code changes before my PRs get merged/handled.

glasserc · 2017-09-29T19:43:10Z

Hmm, I'm not sure I understand what you mean. The templating approach is taken directly from how ArrayField handles ArrayFieldTemplate and the implementation is very similar.

Oops. Somehow I had in my mind the idea that we were less flexible about arrays because of the question in #573. However, looking back at it, I now see that this is about ArrayWidgets, not ArrayFields or their templates, and I had confused myself. OK, great! Let me look at it again.

glasserc

Looks great, just one question.

glasserc · 2017-09-29T20:15:59Z

README.md

+- `content`: The html for the property's content.
+- `name`: A string representing the property name.
+- `disabled`: A boolean value stating if the object property is disabled.
+- `index`: A number stating the index the property occurs in the `properties`.


I still don't really understand why we need to expose this. Knowing that it comes from ArrayField, I can understand why it's valuable there -- but here it seems like the property name is more valuable.

I missed this one... You're right, I removed it.

glasserc · 2017-10-05T20:46:41Z

Thanks!

olzraiti added 4 commits August 1, 2017 15:43

Add ObjectFieldTemplate

a897320

ObjectFieldTemplate properties are representative objects instead of …

2d95ef9

…React elements

merge

5b24519

prettier

f75760b

glasserc reviewed Sep 15, 2017

View reviewed changes

olzraiti mentioned this pull request Sep 24, 2017

Easier Bootstrap Grid usage and basic BS4 compatibility #623

Closed

9 tasks

olzraiti added 3 commits September 25, 2017 10:14

Update README & rename ObjectFieldTemplate props children to content

de3de0c

Update README

1356033

Fix ObjectFieldTemplate test

1213dce

Ran cs-format & cs-check

ad0a40f

glasserc approved these changes Sep 29, 2017

View reviewed changes

killua-eu mentioned this pull request Sep 30, 2017

Revert from forked rjsf with BS4 compat back to upstream. vaizard/glued-archived#38

Closed

olzraiti added 2 commits October 3, 2017 12:35

Remove index from ObjectFieldTemplate props

1e193dd

Ran cs-format & cs-check

890f970

glasserc merged commit 8e9aec5 into rjsf-team:master Oct 5, 2017

glasserc mentioned this pull request Nov 9, 2017

Fix issue #747 #748

Merged

matias-sandell mentioned this pull request Nov 14, 2017

Fieldset preventing flexbox styling #762

Closed

1 task


		The following props are part of each element in `items`:

		- `children`: The html for the property's content.

Uh oh!

Conversation

olzraiti commented Aug 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reasons for making this change

Checklist

Uh oh!

glasserc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olzraiti commented Sep 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glasserc commented Sep 29, 2017

Uh oh!

glasserc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glasserc commented Oct 5, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

olzraiti commented Aug 1, 2017 •

edited

Loading

olzraiti commented Sep 25, 2017 •

edited

Loading