-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Expose title property for validated fields #4700
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
Conversation
params, // specific to ajv | ||
stack, | ||
schemaPath, | ||
title: uiTitle, |
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.
Thanks for the fix... can you add a unit test in utils
to verify this fix? And update the CHANGELOG.md
as well.
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'll try getting something done in the next few days.
Fixed some failing tests after my change and updated the changelog. Still thinking in the best way to make a specific test for the change. |
As the author of #4504, I'm very thankful to see this being taken care of. I have a single proposal/update here, which is also mentioned in #4504. Would it be feasible to expose the entire With the |
@TheOneTheOnlyJJ would it make sense to merge this, and later you (or @lazaromenezes, or another contributor) could enhance the object with the full |
@nickgros if maintainers are okay with merging as it is I'm also okay and address uiOptions in a later pull request and put more work on the tests when exposing the full object. |
The one hesitation I have is that, in adding support for the |
My original thought was adding |
If that is the case, then I'm fine with this merging... Please rebase |
But would it really make sense to expose both It only makes sense when keeping into account that the I hope I'm not unnecessarily delaying the work on this issue, but I'm just trying to keep into account all the details that may impact the usefulness of this feature, based on my experience with having no access to the |
934089a
to
f8123e5
Compare
For this reason, I think it's a good idea to have a top-level
Those other options can be incrementally added to the top level as needed. I don't think it's harmful to also have the Let's merge this. @lazaromenezes thanks for the contribution and @TheOneTheOnlyJJ thanks for your input! |
Reasons for making this change
This exposes the
title
property onRJSFValidationError
to allow further custom message formating.Fixes #4504
In #4504 discussion is mentioned a
rjsf-v6
branch that I couldn't find so I'm targeting main instead. If there's another branch I could retarget this PR please let me know.Checklist
npx nx run-many --target=build --exclude=@rjsf/docs && npm run test:update
to update snapshots, if needed.