-
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
Refactor AltDateWidget to functional component #1522
Conversation
@epicfaace it's a bit unfortunate how the tests are built, since their reliance on asserting on the internal state makes it impossible to use functional components. i suggest that a better solution would be to pass an onChange function in the tests and assert that it has been called with the correct formData. i could help with this if you think it's a good idea. |
@jimmycallin yes, definitely, testing internal state is bad. Your suggestion makes sense -- it would be great if you could do that! If you do do that, could you refactor the tests in a separate PR so we can make sure they still pass on the old code? |
Co-Authored-By: Ashwin Ramaswami <[email protected]>
…tdatewidget-refactor
@epicfaace this one's now (finally!) ready for review! phew. |
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 believe this is a breaking change because clicking on "Now" now adds milliseconds to the value. Is there a particular reason why you added this behavior?
@epicfaace Honestly I don't quite remember, it was a while ago I made this PR. But if you look at this test case: https://github.com/rjsf-team/react-jsonschema-form/blob/master/packages/core/test/StringField_test.js#L926 You can see that currently it is possible to add a datetime with milliseconds as default or through formData, that returns milliseconds on submit. I THINK i thought it was a weird discrepancy between allowing support of milliseconds, but not being able to include them when using the "Now" button. So I chose to include support for them as well. |
@epicfaace Is this still interesting? I understand if you want to keep it consistent to class components since that is what you are using elsewhere, so I don't mind closing this PR if you think so. |
@jimmycallin sorry for the delay. Yeah, I'm a bit hesitant to merge this PR because all the other widgets are class-based and I don't see a huge advantage in moving to a functional component. I think it would be great to, at least for now, try to remove componentWillReceiveProps from the class-based component, though. |
Thought I could help you with updating a component that still relied on componentWillReceiveProps :)
Reasons for making this change
If this is related to existing tickets, include links to them as well.
Checklist