-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Make error_class work on inner wrapper #1729
base: main
Are you sure you want to change the base?
Conversation
@carlosantoniodasilva If you have some time to look at this, it would be greatly appreciated! |
@rafaelfranca If you have some time to look at this, it would be greatly appreciated! |
@lafeber I did get your initial ping and should've commented back that'd get to it here, that was my bad, but please be a little patient. :) Thanks! |
@carlosantoniodasilva I'm truly sorry, I didn't mean to be impatient. I appreciate all the work you put in this gem - it's one of my all time favourites. |
Hello everyone, I was wondering if you plan to merge this as we're also facing this issue. Thanks |
This change solves some issues I was having as well, would be great if it was merged in! |
lib/simple_form/wrappers/many.rb
Outdated
@@ -67,7 +67,9 @@ def html_options(options) | |||
end | |||
|
|||
def html_classes(input, options) | |||
@defaults[:class].dup | |||
css = @defaults[:class].dup | |||
css << @defaults[:error_class].dup { input.has_errors? } |
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.
css << @defaults[:error_class].dup { input.has_errors? } | |
css << @defaults[:error_class] if input.has_errors? |
Aslo if there's no error class setup on wrapper then there's no need to check for errors.
@rafaelfranca @lafeber I updated this PR with a fix to how we check for input's errors. Previously it wasn't properly checking it so my newly added spec was failing. I fixed that. @rafaelfranca @carlosantoniodasilva if PR looks good to you I'll merge it and will update Changelog/README afterward. |
Fix for #1727