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 small fixes for bootstrap 3. Add tests. #85

Open
wants to merge 23 commits into
base: master
Choose a base branch
from
Open

Add small fixes for bootstrap 3. Add tests. #85

wants to merge 23 commits into from

Conversation

schneikai
Copy link

Hi,

I use this gem in a current Rails 4 project so I added some fixes to make it work better with Bootstrap 3. I also added tests for everything.

Details:
I removed the example code for inline. Correct me if I'm wrong but this wasn't TWBS 3 markup. Inline form elements in horizontal forms are also not mentioned in the Bootstrap docs so maybe we should remove this? If it should stay, I think the right markup would be to add a row container and add a column class to every form-group inside that row.

Also the date_select won't render 3 fields on one line without extra markup in TWBS 3 so the old screenshot is wrong. We would need to add extra css to make the 3 select inputs float or wrap them with a row and add column classes like for inline inputs.

Updated the height sizing classes to use input-sm instead of small.

Added the select element styling fix from #78

Updated the screenshot.

test/dummy/db/*.sqlite3-journal
test/dummy/log/*
test/dummy/tmp/*
test/dummy/.sass-cache
Copy link
Owner

Choose a reason for hiding this comment

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

Please use a project-specific .gitignore. Most of the entries in this seem to be specific to your development environment, and should be in ~/.gitignore.

@stouset
Copy link
Owner

stouset commented Nov 19, 2013

The example screenshot no longer has inline form fields, as it does in the original example image.

@@ -1,7 +1,7 @@
require 'twitter_bootstrap_form_for'
require 'action_view/helpers'

class TwitterBootstrapFormFor::FormBuilder < ActionView::Helpers::FormBuilder
class TwitterBootstrapFormFor::TwitterBootstrapFormFor::FormBuilder < ActionView::Helpers::FormBuilder
Copy link
Owner

Choose a reason for hiding this comment

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

Why the double-nesting of the namespace?

@stouset
Copy link
Owner

stouset commented Nov 19, 2013

Outside of the comments, it looks good. Thanks.

@schneikai
Copy link
Author

About the inline form fields:
I removed the example code for inline. Correct me if I'm wrong but this wasn't TWBS 3 markup. Inline form elements in horizontal forms are also not mentioned in the Bootstrap docs so maybe we should remove this? If it should stay, I think the right markup would be to add a row container and add a column class to every form-group inside that row.

I could add it like this if you want to.

@stouset
Copy link
Owner

stouset commented Nov 22, 2013

If we can keep some way to have inline form fields, it would be great. Use your best judgment, though — if TB doesn't support anything like it now, and it looks like any approach would be likely to break in future updates, we should probably abandon it.

@stouset stouset mentioned this pull request Nov 23, 2013
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.

2 participants