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

Skip valid_class check if no class defined #1657

Conversation

TALlama
Copy link
Contributor

@TALlama TALlama commented Apr 23, 2019

#1656

Previously, we would check for validity all the time, even if the outcome of the check added no class. This meant that there was no way to opt out of the check in cases where it was expensive (if the file is stored in S3, in our example).

Now, we only check for validity if there is a valid_class defined to be added if the field is valid.

heartcombo#1656

Previously, we would check for validity all the time, even if the outcome of the check added no class. This meant that there was no way to opt out of the check in cases where it was expensive (if the file is stored in S3, in our example).

Now, we only check for validity if there is a valid_class defined to be added if the field is valid.
@TALlama
Copy link
Contributor Author

TALlama commented May 8, 2019

Anything I can do to expedite review? This issue is currently blocking us from updating simple_form, which is blocking us from updating to Rails 5.2.

@@ -59,6 +59,15 @@ class WrapperTest < ActionView::TestCase
assert_no_select 'input.is-invalid'
end

test 'wrapper does not determine if valid class is needed when it is set to nil' do
Copy link
Collaborator

@feliperenan feliperenan Jan 22, 2020

Choose a reason for hiding this comment

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

This test is not covering your changes - I put back the deleted code and it still pass. One way I can think right now in how to test it, would be with something like:

  test 'wrapper does not determine if valid class is needed when it is set to nil' do
    @user.instance_eval { undef errors }
    with_form_for @user, :name, wrapper: custom_wrapper_with_input_valid_class(valid_class: nil)

    assert_no_select 'div.field_without_errors'
  end

A better test would checking if input.valid? has not been called at all, but I don't know how to do it 🙈

@feliperenan
Copy link
Collaborator

Hi @TALlama

Thanks for your contribution and sorry about the delay 🙌 . Before we decide whether or not to move forward with this, I'd like to know if you are still facing this issue or did you work around it somehow?

@feliperenan feliperenan merged commit ac5759c into heartcombo:master Feb 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants