-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Partially fixed dependency error messages #4417
Conversation
…:title. This fix only applicable if we use an ajv-i18n localizer. Ref. rjsf-team#4402.
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.
For the most part this looks good. I've requested a few documentation comments as well as left you a question
if ('missingProperty' in params) { | ||
property = property ? `${property}.${params.missingProperty}` : params.missingProperty; | ||
} | ||
|
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.
Is this still needed since you are always adding missingProperty
to rawPropertyNames
above?
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 need this if clause as params.missingProperty
may be undefined. If params.missingProperty
is undefined, it is removed from rawPropertyNames
by filter((item) => item)
.
const uiSchemaPath = schemaPath | ||
.replace(/\/properties\//g, '/') | ||
.split('/') | ||
.slice(1, -1) | ||
.concat([currentProperty]); |
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.
Can you provide a comment that simply describes what these chained statements are accomplishing?
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.
Sure. I added a comment as follows:
To retrieve a title from UI schema, construct a path to UI schema from `schemaPath` and `currentProperty`.
For example, when `#/properties/A/properties/B/required` and `C` are given, they are converted into `['A', 'B', 'C']`.
} | ||
}); | ||
if (error.params?.deps) { | ||
error.params.deps = error.params.deps |
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.
Can you provide a comment that simply describes what these chained statements are accomplishing?
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.
I added a comment as follows:
As `error.params.deps` is the comma+space separated list of missing dependencies, enclose each dependency separately.
For example, `A, B` is converted into `'A', 'B'`.
error.params.deps = error.params.deps | ||
.split(', ') | ||
.map((v: string) => v.slice(1, -1)) | ||
.join(', '); |
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.
Can you provide a comment that simply describes what these chained statements are accomplishing?
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.
I added a comment as follows:
Remove surrounding quotes from each missing dependency. For example, `'A', 'B'` is reverted to `A, B`.
Reasons for making this change
Partially fixed issue where dependency errors do not show title or ui:title. This fix only applicable if we use an ajv-i18n localizer. Ref. #4402.
Checklist
npx nx run-many --target=build --exclude=@rjsf/docs && npm run test:update
to update snapshots, if needed.