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

invalid-feedback should be inside input-group #417

Closed
colinbm opened this issue Jan 29, 2018 · 7 comments · Fixed by #434
Closed

invalid-feedback should be inside input-group #417

colinbm opened this issue Jan 29, 2018 · 7 comments · Fixed by #434
Assignees
Milestone

Comments

@colinbm
Copy link

colinbm commented Jan 29, 2018

The style that triggers display of the error is (amongst others) .form-control.is-invalid ~ .invalid-feedback.

Currently the .invalid-feedback div is outside the .input-group and as .is-valid is set on the input itself, it's never displayed.

This has been addressed in twbs/bootstrap#25020

Added support for form validation feedback in input groups. .input-group now has flex-wrap: wrap; on by default, allowing you to place validation feedback within the .input-group and style it based on the input's overall state. This isn't applicable to file inputs though.

This means the .invalid-feedback should be located as the last item inside .input-group.

I will take a look at this myself and see if I can fix, but it's probably an easy fix for someone who knows the code better. Thanks if you get there first!

@lcreid
Copy link
Contributor

lcreid commented Jan 29, 2018

A big thank you for taking the time to report this. And thanks for offering to take a look at a fix. I encourage you to have a go at submitting a PR. We really like to get first-time contributions. Post comments here if you need help and I'll try to give a hand. I'm pretty new to the code myself. Take a look at the CONTRIBUTING document if you haven't already.

As you know if you've read #361, we're in the middle of trying to prepare an official release of bootstrap_form for Bootstrap 4, so we're all pretty busy. And this is a completely community-supported gem. So your help would be really appreciated, but don't feel pressured if you can't.

@mattbrictson mattbrictson added this to the 4.0.0 milestone Jan 31, 2018
@lcreid
Copy link
Contributor

lcreid commented Feb 3, 2018

Fixing this would move me towards being able to finish #410 and #418, so I'll take a stab at a PR. Let me know if that's a bad thing.

@lcreid lcreid self-assigned this Feb 3, 2018
@colinbm
Copy link
Author

colinbm commented Feb 3, 2018

Go for it. I have got as far as writing a test for it, but my prospective fix seems... very wrong. (Regex replace after the html is generated)

@lcreid
Copy link
Contributor

lcreid commented Feb 3, 2018

I'd love to get your test case, if you can push it to your fork and send me a link. Or even just cut and paste it into a comment here.

@colinbm
Copy link
Author

colinbm commented Feb 4, 2018

Will do tomorrow. Away from computer at the moment.

lcreid added a commit to lcreid/rails-bootstrap-forms that referenced this issue Feb 4, 2018
lcreid added a commit to lcreid/rails-bootstrap-forms that referenced this issue Feb 4, 2018
@lcreid
Copy link
Contributor

lcreid commented Feb 4, 2018

@colinbm take a look at PR #422, if you have a chance. Let me know what you think. I'll add your test case when I get it.

@colinbm
Copy link
Author

colinbm commented Feb 4, 2018

@lcreid Looks good. You appear to have written the exact same test already! :)

colinbm@f035bf2

mattbrictson pushed a commit that referenced this issue Feb 23, 2018
* Test case for #417.

* Undoing mess created on wrong branch.

* Remove test that accidentally leaked in from another branch.

* Working but needs cleanup #417.

* Partially cleaned up.
Added method for error and help and optionally prepend and append.

* Remove unneeded method and parameters.

* Test selects error.

* Test selects error.

* time_zone_select works for new error placement.

* Add tests for date, time, datetime selects.

* Add tests for date, time, datetime selects.

* date, time, datetime tests pass with old implementation.

* Date, time, and datetime for new implementation.

* Tests for file and collection selects.

* File and collection selects with new implementation.

* Keep tests but revert lib to try another approach.

* form_group tests passing.

* Clean up and add back commented test.

* checkbox and radio button tests still have old behaviour.

* Clean up and block append and prepend.
lcreid added a commit to lcreid/rails-bootstrap-forms that referenced this issue Jun 4, 2018
* Test case for bootstrap-ruby#417.

* Undoing mess created on wrong branch.

* Remove test that accidentally leaked in from another branch.

* Working but needs cleanup bootstrap-ruby#417.

* Partially cleaned up.
Added method for error and help and optionally prepend and append.

* Remove unneeded method and parameters.

* Test selects error.

* Test selects error.

* time_zone_select works for new error placement.

* Add tests for date, time, datetime selects.

* Add tests for date, time, datetime selects.

* date, time, datetime tests pass with old implementation.

* Date, time, and datetime for new implementation.

* Tests for file and collection selects.

* File and collection selects with new implementation.

* Keep tests but revert lib to try another approach.

* form_group tests passing.

* Clean up and add back commented test.

* checkbox and radio button tests still have old behaviour.

* Clean up and block append and prepend.
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 a pull request may close this issue.

3 participants