Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

MichalRemis
Copy link
Contributor

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.

@coveralls
Copy link

coveralls commented Apr 21, 2020

Coverage Status

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

Copy link
Contributor

@tagliala tagliala left a 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

lib/client_side_validations/action_view/form_builder.rb Outdated Show resolved Hide resolved
src/main.js Outdated Show resolved Hide resolved
test/action_view/cases/test_form_for_helpers.rb Outdated Show resolved Hide resolved
test/action_view/cases/test_form_for_helpers.rb Outdated Show resolved Hide resolved
test/javascript/public/test/validateElement.js Outdated Show resolved Hide resolved
src/main.js Outdated Show resolved Hide resolved
test/javascript/public/test/validateElement.js Outdated Show resolved Hide resolved
test/javascript/public/test/validateElement.js Outdated Show resolved Hide resolved
test/javascript/public/test/validateElement.js Outdated Show resolved Hide resolved
@MichalRemis
Copy link
Contributor Author

Thanks for feedback. All requested changes are fixed.

Comment on lines +251 to +253
// 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
Copy link
Contributor

@tagliala tagliala Apr 28, 2020

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

Copy link
Contributor Author

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)

@tagliala
Copy link
Contributor

@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 added a commit to tagliala/csv-playground that referenced this pull request May 1, 2020
@tagliala
Copy link
Contributor

tagliala commented May 1, 2020

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

@MichalRemis
Copy link
Contributor Author

MichalRemis commented May 2, 2020

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 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 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/

Base automatically changed from master to main February 13, 2021 10:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants