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

Use field specific wrappers #81

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

Conversation

MichalRemis
Copy link

Implemented field's specific wrapper_names to be passed to and used by CSV JS FormBuilder for adding/removing of error messages. Before CSV used only form-wide wrapper but SimpleForm's field may use custom wrapper specified:

  • by field's wrapper attribute
  • by field's type and wrapper_mappings attribute
  • by field's type and SimpleForm.config.wrapper_mappings

I think this feature will be also required for implementation of validating of radios/dates/etc.

I am passing wrapper_name by input's data-simple-form-wrapperattribute. Was thinking about putting it into CSV Hash, but I think it would require CSV code change and this is SimpleForm's thing anyway.

@coveralls
Copy link

coveralls commented Apr 17, 2020

Coverage Status

Coverage remained the same at 100.0% when pulling 00950a5 on MichalRemis:UseFieldSpecificWrappers into 2638bf2 on DavyJonesLocker:master.

@tagliala tagliala self-requested a review April 17, 2020 12:50
@tagliala
Copy link
Contributor

I think we can ignore the codeclimate offences

@tagliala
Copy link
Contributor

tagliala commented Apr 25, 2020

Please rebase on the current master, this PR is not clean

If you have troubles in rebase, let me know

Short-not-tested rebase guide

git checkout master
git add upstream https://github.com/DavyJonesLocker/client_side_validations-simple_form.git
git pull upstream master # now your master branch is up-to-date with upstream
git checkout UseFieldSpecificWrappers
git rebase -- master

Follow on screen instructions, then:

git push -f

After the rebase, I should not see anymore the following commits in this PR:

image

@MichalRemis MichalRemis force-pushed the UseFieldSpecificWrappers branch from 1d5da93 to 00950a5 Compare April 25, 2020 21:01
@MichalRemis
Copy link
Author

ok, rebased

@tagliala
Copy link
Contributor

@MichalRemis thanks! Now it looks good

…y CSV Javascript form builder for adding/removing of error messages. Before CSV used only form-wide wrapper but field may use custom wrapper specified:

    - by field's `wrapper` attribute
    - by field's type and `wrapper_mappings` attribute
    - by field's type and SimpleForm config `wrapper_mappings`
@MichalRemis MichalRemis force-pushed the UseFieldSpecificWrappers branch from 00950a5 to 3996907 Compare April 28, 2020 17:44
@@ -22,6 +22,8 @@ def input(attribute_name, options = {}, &block)
options.delete(:validate)
end

add_field_specific_wrapper_name_to_field_options(attribute_name, options, &block)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving this to the parent gem will not be straightforward. I think I should add the method call to all the overrides

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I agree

Base automatically changed from master to main January 23, 2021 08:39
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.

3 participants