-
Notifications
You must be signed in to change notification settings - Fork 403
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
Add support for date/time/date_time selects #784
base: main
Are you sure you want to change the base?
Add support for date/time/date_time selects #784
Conversation
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 this PR.
This is just a partial review, some other things need to be clarified but I do not have much time
6ddafe4
to
d62a26d
Compare
Thanks for feedback. All requested changes are fixed. |
b02cf72
to
5fa9a94
Compare
// showing validation messages doesnt work well with this. | ||
// JS Formbuilder must be customized for these types of fields | ||
// to share error message and hide error only when all 3 selects are valid |
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.
This will require attention. More details in the main PR
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 know. But I am focused on getting CSV-simple_form work. Not sure if I will have time for this here. I just put it into main, because it was missing here and believed it won't break anything (because time/date_selects wasn't working before anyway)
@MichalRemis I want to thank you for your work and apologize because I do not have too much time during the week to review this change. During the weekend I will share a demo rails application with CSV to test stuff and add you permissions. |
Hi @MichalRemis , as promised I've created a new repo for testing PR/issues: https://github.com/tagliala/csv-playground Branch https://github.com/tagliala/csv-playground/tree/client-side-validations/784 I've investigated this issue and I think that a proper implementation should focus on how validation errors are being detected, instead of how to display them in multi select fields. I think that the following could be a proper behavior:
Point 1 suggests that the validation should trigger only on certain circumstances: probably CSV should manage Point 2 gives us a hint on when to run the validation add the error, but it also suggests that we should check the element with the focus after a blur occurs... and I do not see anything good here Hopefully, we can get the 3rd point for free, if we manage to properly detect errors and ask CSV to add the error to the nth-element of the multi select |
5fa9a94
to
06ce081
Compare
Ok I checked the playground app and I think it will be quite a challenge to to implement this in main CSV, because, unlike I can imagine, (after implementing DavyJonesLocker/client_side_validations-simple_form#81) that custom default wrapper could be set to date_selects etc., with special add/remove functions to achieve desired behaviour. But it won't be straightforward because of Due to these limitations I am not sure now, if it makes sense to put these special fields into mainCSV. I can put it all into into CSV-simple_form only, without need for changes in mainCSV what do you think? You can test all new fields from my simple_form's PRs here http://kamnavylet.sk:3000/ |
Added support new fields, which weren't overridden by CSV form_builder and therefore not included in CSV Hash.
Showing of error_message doesn't work well with these, because of special naming and logic (multiple select should share one message) of this fields and would require customizing JS FormBuilder add/remove functions - this would be easier once you transfer JS FormBuilder's
wrappers:
from my CSV_simple_form PR DavyJonesLocker/client_side_validations-simple_form#81.Anyway, at least validations are in CSV hash now, and input's are being marked as invalid and this PR is also needed to make date/time inputs work in CSV_simple_form
There is again one rubocop offense
Metrics/ModuleLength: Module has too many lines
for form_builder.rb, I leave this to your decision.