-
Notifications
You must be signed in to change notification settings - Fork 376
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
React material table cell validationMode #2400
base: master
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for jsonforms-examples ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
); | ||
const messageFormHelperText = wrapper.find(FormHelperText).at(0); | ||
expect(messageFormHelperText.text()).toBe( | ||
"must have required property 'message'" |
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.
@sdirix Thought I'd at least attempt to add relevant unit tests - there didn't seem to be any for MaterialTableControl
This fails - not sure if validation just doesn't run for some reason
If you know why let me know and I'll fix it, otherwise I can either skip this test, or remove this file all together.
Testing manually with the examples seems to be working as expected
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.
Interestingly, the test works with message: { type: 'string', minLength: 3 }
So that's another option, but doesn't really explain why required is not being calculated correctly in this context
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.
In the unit test, did you add the required property as an empty string or leave out the property completely?
If it's empty, the required check does not fail. In the ui, the renderer unsets the field to undefined when it is empty. That could be a reason for different results.
I also tested it in the example app and there showing the required error on an array item still worked fine (I locally extended the arrays.ts example for this).
Could you re-add a unit test for this? If it still doesn't run as expected, I can have a look then :)
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.
You're absolutely right, I've added more test cases to reflect this
811073d
to
d8371c2
Compare
@0mpurdy Thanks for the contribution ❤️ |
d8371c2
to
0038727
Compare
Fix #2398