Skip to content

Conversation

amarnus
Copy link
Contributor

@amarnus amarnus commented Jul 6, 2016

Helps with #218

return (
<div className={classList}>
{displayLabel && label ? getLabel(label, required, id) : null}
<DescriptionField id={ `${id}__description` } description={ description } />
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: {${id}__description} (no space within curly brackets)

@n1k0
Copy link
Collaborator

n1k0 commented Jul 6, 2016

Would you mind adding a few tests? Thanks!

@n1k0 n1k0 changed the title Allow rendering description from schema for all fields. Fixes #218: Allow rendering description from schema for all fields. Jul 6, 2016
@amarnus amarnus force-pushed the dev-description branch from b6f2063 to 56bbfef Compare July 6, 2016 07:25
@amarnus
Copy link
Contributor Author

amarnus commented Jul 6, 2016

Done.

const schema = {
type: "object",
properties: {
foo: {type: "string",description:"A Foo field"},
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: missing space after comma

@amarnus amarnus force-pushed the dev-description branch from 56bbfef to 708b390 Compare July 6, 2016 07:30
@amarnus
Copy link
Contributor Author

amarnus commented Jul 6, 2016

Thanks for highlighting the styling issues. Resolved now. npm run lint didn't catch them though. Will be good to add these rules in there, I feel?

const schema = {
type: "object",
properties: {
foo: {"$ref": "#/definitions/foo"},
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: $ref (no quotes) works too

@n1k0
Copy link
Collaborator

n1k0 commented Jul 6, 2016

npm run lint didn't catch them though. Will be good to add these rules in there, I feel?

If you know about what the rules to add are, please add them :)

@amarnus
Copy link
Contributor Author

amarnus commented Jul 6, 2016

If you know about what the rules to add are, please add them :)

Will do. As another pull request.

@amarnus amarnus force-pushed the dev-description branch from 708b390 to 95c53b9 Compare July 6, 2016 07:40

it("should render description if available from a referenced schema", () => {
// Overriding.
const schema = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm surprised you can reassign a const here. You should probably name this specific schema differently instead.

@n1k0
Copy link
Collaborator

n1k0 commented Jul 6, 2016

Side note, please avoid squashing and keep your commits atomic on the PR so I can review them individually. Squashing is enabled for this repo so a single commit will eventually land anyway :)

@amarnus
Copy link
Contributor Author

amarnus commented Jul 6, 2016

Noted. It's just that I didn't want the intermediate commits on my fork either. But seeing that you have left comments on them, I guess I shouldn't be squashing them. Got it.

@amarnus
Copy link
Contributor Author

amarnus commented Jul 6, 2016

When you have a spare moment later, can restart the Travis build? It failed because of issues with npm.

@n1k0
Copy link
Collaborator

n1k0 commented Jul 6, 2016

Jobs restarted. Edit: restarted again. Edit: npm's broken today it seems.

bar: {type: "string"}
}
};
it("should render description if available from the schema", () => {
Copy link
Collaborator

@n1k0 n1k0 Jul 6, 2016

Choose a reason for hiding this comment

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

nit: insert blank line above this line

@n1k0
Copy link
Collaborator

n1k0 commented Jul 6, 2016

Strange issue with travis it seems, where fetching this url https://registry.npmjs.org/babel-plugin-transform-object-rest-spread/-/babel-plugin-transform-object-rest-spread-6.8.0.tgz fails with a 502 while

$ curl -I https://registry.npmjs.org/babel-plugin-transform-object-rest-spread/-/babel-plugin-transform-object-rest-spread-6.8.0.tgz
HTTP/1.1 200 OK

@n1k0
Copy link
Collaborator

n1k0 commented Jul 6, 2016

Perfect, I'm landing this. Again, thanks for contributing!

@n1k0 n1k0 merged commit 278be64 into rjsf-team:master Jul 6, 2016
n1k0 added a commit that referenced this pull request Jul 6, 2016
* Fixed #217: Implement array field entries reordering (#247)
* Fixed corrupted `idSchema` when the schema has a field named id (#262)
* Fixed #218: Allow rendering description from schema for all fields. (#263)
* Fixed #255: Replace *deeper* dependency with local `deepEquals` helper. (#264)
@n1k0
Copy link
Collaborator

n1k0 commented Jul 6, 2016

Released in v0.36.0.

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.

2 participants