-
Notifications
You must be signed in to change notification settings - Fork 225
Feature/add title #871
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
Feature/add title #871
Conversation
libV2/schemaUtils.js
Outdated
| utils.addToJsonPath(currentPath, [compositeKeyword, index])); | ||
| }) }; | ||
| return { | ||
| title: schema.title, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should only add title and description fields if they have a valid value.
Let's do this for all other places where we're adding title and description
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it doesn't affect the type information. But you're adding unnecessary fields into the schema object that is passed and used everywhere in the module. The undefined fields are not just added for the type information.
Which I'm trying to avoid.
libV2/schemaUtils.js
Outdated
| */ | ||
| if (resolveFor === TYPES_GENERATION) { | ||
| return { | ||
| title: schema.title, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't we add all of the content from schema object ?
| title: schema.title, | |
| ...schema |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, but since spread of objects wasn't supported here (due to some problem related to ES version required vs present in .eslintrc), i used it explicitly which i felt to be fine as well 😅 .
| createProperties = (param) => { | ||
| const { schema } = param; | ||
| return { | ||
| description: schema.description, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: Should we add title and description to the param fields ? How does this affect the types UI in the collection for the params ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.


Added title and description to type schema for 2-way-sync