client_side_validations icon indicating copy to clipboard operation
client_side_validations copied to clipboard

Add support for date/time/date_time selects

Open MichalRemis opened this issue 4 years ago • 5 comments

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 https://github.com/DavyJonesLocker/client_side_validations-simple_form/pull/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.

MichalRemis avatar Apr 21 '20 21:04 MichalRemis

Coverage Status

Coverage remained the same at 100.0% when pulling 06ce081da5a78cc8cf4f5fc2d15ccb28aa8dbaf1 on MichalRemis:SupportDateTimeSelects into a49dd74a14b259b9a08551fedd163cf2406ba25c on DavyJonesLocker:master.

coveralls avatar Apr 21 '20 23:04 coveralls

Thanks for feedback. All requested changes are fixed.

MichalRemis avatar Apr 26 '20 10:04 MichalRemis

@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.

tagliala avatar Apr 28 '20 20:04 tagliala

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:

  1. Error should not appear while I'm filling multiple fields of the same select (eg: fill in day of date_of_birth, press TAB, should not get an error because month and year in date_of_birth are still empty).
  2. Error should appear when I change focus from a multi select to another field (eg: fill in day of date_of_birth, change focus to name);
  3. Only one error should be displayed for a multi-select field

Point 1 suggests that the validation should trigger only on certain circumstances: probably CSV should manage 1i, 2i, ni as they are the same field.

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

tagliala avatar May 01 '20 11:05 tagliala

Ok I checked the playground app and I think it will be quite a challenge to to implement this in main CSV, because, unlike simple_form, it doesn't use wrappers nor error class, so we don't even know, that we are validating these special (datetime/checkboxes) field nor we don't know if sibling inputs are invalid.

I can imagine, (after implementing https://github.com/DavyJonesLocker/client_side_validations-simple_form/pull/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 field_error_proc approach of mainCSV.

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/

MichalRemis avatar May 02 '20 21:05 MichalRemis