Skip to content

Bootstrap v4#299

Closed
antpaw wants to merge 19 commits intobootstrap-ruby:bootstrap-v4from
antpaw:bootstrap-v4
Closed

Bootstrap v4#299
antpaw wants to merge 19 commits intobootstrap-ruby:bootstrap-v4from
antpaw:bootstrap-v4

Conversation

@antpaw
Copy link
Copy Markdown
Contributor

@antpaw antpaw commented Oct 20, 2016

This branch can be used for all the changes that will come with the update to v4

@mattbrictson
Copy link
Copy Markdown
Contributor

Thanks for contributing this! Is this PR a work in progress, or is it ready for review?

If it is not ready yet, could you add "WIP" to the PR name?

@antpaw
Copy link
Copy Markdown
Contributor Author

antpaw commented Oct 21, 2016

Hi, I'm building a rails CRUD app with bootstrap_form from my repo and 'bootstrap', '~> 4.0.0.alpha4' and I can't detect any problems that would prevent from merging. However I wouldn't release a full major version of this, only a beta, because v4 is in alpha now.

… checkbox labels

* Switched wrapper classes for checkboxes from "checkbox" to "form-check" for Boostrap v4

* Added form-check-input class to input types of 'checkbox' type for Boostrap v4

* Added form-check-label to form-labels, but not to inline-form-label's because they're not supported yet. Bootstrap v4 fixes.

* Updated CHANGELOG.md to include info about checkboxes for Bootstrap v4
@desheikh
Copy link
Copy Markdown
Collaborator

desheikh commented Jan 18, 2017

@antpaw ditto, looks like this work's already in the bootstrap-v4 branch

@bradlis7
Copy link
Copy Markdown

Personally, I feel that since the bootstrap gem is keeping a v4 alpha up to date, it would be useful if we could get this as a gem as well (maybe require specifying the version if possible). Can we merge the changes from master into this branch as well?

…p-v4

# Conflicts:
#	test/bootstrap_checkbox_test.rb
#	test/bootstrap_fields_test.rb
#	test/bootstrap_form_group_test.rb
#	test/bootstrap_form_test.rb
#	test/bootstrap_selects_test.rb
@antpaw
Copy link
Copy Markdown
Contributor Author

antpaw commented Feb 13, 2017

ok, just did it

@bradlis7
Copy link
Copy Markdown

Here's how you can use this branch in a project:

// Gemfile
gem 'bootstrap_form', git: 'https://github.com/bootstrap-ruby/rails-bootstrap-forms.git', branch: 'bootstrap-v4'

@antpaw
Copy link
Copy Markdown
Contributor Author

antpaw commented Feb 15, 2017

@bradlis7 this branch is not my branch, they have started their own for some reason.

@antpaw antpaw changed the base branch from master to bootstrap-v4 February 15, 2017 09:25
@antpaw antpaw changed the base branch from bootstrap-v4 to master February 15, 2017 09:25
@bradlis7
Copy link
Copy Markdown

Well, that branch is somehow associated to this PR, as it says "View #299" when I open that branch. I assumed they pulled some of your changes in. It seems like there are about 5 or 6 people with changes for the conversion, so it's a little confusing.

@mattbrictson
Copy link
Copy Markdown
Contributor

Hi, this repo has an official bootstrap-v4 branch that we are using to consolidate all v4-related PRs. This is so we can review and merge improvements to v4 support incrementally without disrupting the master branch.

However we don't yet have an official PR to go with the bootstrap-v4 branch, hence why people are getting directed here. I will create an official PR for v4 to clear up the confusion.

@mattbrictson
Copy link
Copy Markdown
Contributor

Here is the official bootstrap-v4 PR: #315

@antpaw Have all your commits already been included in #315? If so can we close this PR?

@antpaw
Copy link
Copy Markdown
Contributor Author

antpaw commented Feb 15, 2017

@mattbrictson ye they both look similar, it would be easier to review if your v4 branch also had the newest master in it..

@mattbrictson
Copy link
Copy Markdown
Contributor

@antpaw good point; I have now rebased it on the latest master.

@antpaw antpaw changed the base branch from master to bootstrap-v4 February 16, 2017 08:38
@antpaw
Copy link
Copy Markdown
Contributor Author

antpaw commented Feb 16, 2017

hmm not sure why the diff is still so big, https://github.com/bootstrap-ruby/rails-bootstrap-forms/pull/299/files I like my version better e.g. the

tag for form errors and the def error_class

@mattbrictson
Copy link
Copy Markdown
Contributor

@desheikh and @donv, since you've helped review v4 PRs before, would either of you mind taking a look at this one?

@antpaw Thanks for the PR and for changing the merge target to bootstrap-ruby:bootstrap-v4. Could you resolve the conflicts so it is easier to review?

@desheikh
Copy link
Copy Markdown
Collaborator

@mattbrictson @antpaw I think the reason the diff is so large is because this branch needs to be rebased against the bootstrap-v4 branch. Most of these changes already exist on bootstrap-v4.

@antpaw
Copy link
Copy Markdown
Contributor Author

antpaw commented Feb 17, 2017

hmm it looks like github is bugged, the target branch is bootstrap-ruby:bootstrap-v4 but i still see diffs like this one https://abload.de/img/screenshot2017-02-17a4rrb2.png but this should not have any diff if you look at the current state of the two branches

https://github.com/bootstrap-ruby/rails-bootstrap-forms/blob/bootstrap-v4/lib/bootstrap_form/form_builder.rb#L268
https://github.com/antpaw/rails-bootstrap-forms/blob/master/lib/bootstrap_form/form_builder.rb#L268

i will open an new pull request

edit: nope, new pull request wont help, i still see this nonesens diff, i will try to merge your v4 branch into this one

Anton Pawlik added 3 commits February 17, 2017 11:38
…p-v4

# Conflicts:
#	README.md
#	lib/bootstrap_form/form_builder.rb
#	test/bootstrap_checkbox_test.rb
#	test/bootstrap_fields_test.rb
#	test/bootstrap_form_group_test.rb
#	test/bootstrap_form_test.rb
#	test/bootstrap_radio_button_test.rb
#	test/bootstrap_selects_test.rb
@antpaw
Copy link
Copy Markdown
Contributor Author

antpaw commented Feb 17, 2017

okay i have merged v4 into my v4

@antpaw
Copy link
Copy Markdown
Contributor Author

antpaw commented Feb 17, 2017

one of the main changes is now that i have switched to a <p> tag for inline error and to a <div> for help text. this is done because this is how the documentation requires it. https://v4-alpha.getbootstrap.com/components/forms/?#help-text <span> tag is not a block element. The help text is not <p> because if you stack two <p>s (help and error) on top of each other it looks bad because of the padding.

Comment thread lib/bootstrap_form/form_builder.rb Outdated
label = generate_label(options[:id], name, options[:label], options[:label_col], options[:layout]) if options[:label]
control = capture(&block).to_s
control.concat(generate_help(name, options[:help]).to_s)
# TODO create `generate_error`
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

remove todo tag

@desheikh
Copy link
Copy Markdown
Collaborator

@mattbrictson @antpaw need to remove the #TODO comment, looks good otherwise!

@antpaw
Copy link
Copy Markdown
Contributor Author

antpaw commented Feb 26, 2017

done

@antpaw
Copy link
Copy Markdown
Contributor Author

antpaw commented Feb 26, 2017

please note my other pr that covers this todo #300

@desheikh desheikh added this to the 4.0.0 milestone Jan 11, 2018
@desheikh
Copy link
Copy Markdown
Collaborator

@antpaw @mattbrictson closing this since the changes in beta3 make this obsolete.
#370 covers the new syntax.

@desheikh desheikh closed this Jan 12, 2018
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.

5 participants