Skip to content

Update layout: inline at field level for Bootstrap 4.#433

Merged
mattbrictson merged 13 commits intobootstrap-ruby:masterfrom
lcreid:426-layouts
Feb 23, 2018
Merged

Update layout: inline at field level for Bootstrap 4.#433
mattbrictson merged 13 commits intobootstrap-ruby:masterfrom
lcreid:426-layouts

Conversation

@lcreid
Copy link
Copy Markdown
Contributor

@lcreid lcreid commented Feb 17, 2018

For layout: :inline elements to look right, Bootstrap 4 requires some markup at a "lower level" of the HTML than Bootstrap 3 did. This PR ensures that the elements correctly calculate whether the developer has requested in-line layout, and applies the appropriate classes.

In some cases, classes for in-line layout are applied regardless of whether the layout was specified at the form level or the field level. In other cases, the markup is only needed and applied if the in-line layout was only specified at the field level.

This PR adds a number of test cases for the six variations of form-level and field-level layout being different. The test cases test a few different types of fields:

  • A text-like input, since a large number of types of input fields are handled internally like a text input
  • A select, because selects are hendled differently
  • Check box and radio button, since those two are handled very differently from the rest, and in fact are subtly different between the two
  • A collection radio button, since it adds some additional handling not used by any of the above

I think it's important to have this level of test coverage, even if it could be argued that some of this might be tested elsewhere. (I thought that the date/time selects were similar enough to the others to forgo a test.)

There will be two outstanding issues after this PR is merged:

Fixes #426.
Fixes #416.

@bootstrap-ruby-bot
Copy link
Copy Markdown

bootstrap-ruby-bot commented Feb 17, 2018

1 Warning
⚠️ Please update CHANGELOG.md with a description of your changes. If this PR is not a user-facing change (e.g. just refactoring), you can disregard this.

Here's an example of a CHANGELOG.md entry (place it immediately under the * Your contribution here! line):

* [#433](https://github.com/bootstrap-ruby/bootstrap_form/pull/433): Update `layout: inline` at field level for Bootstrap 4. - [@lcreid](https://github.com/lcreid).

Generated by 🚫 Danger

@mattbrictson mattbrictson added this to the 4.0.0 milestone Feb 17, 2018
Copy link
Copy Markdown
Contributor

@mattbrictson mattbrictson left a comment

Choose a reason for hiding this comment

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

Thanks for putting this PR together.

These changes seem OK to me, but I don't really understand many of the tests. Do the tests represent real-world scenarios? When I copy the expected HTML in to bootply.com to preview, the rendered forms in many cases look undesirable.

I guess my question is: just because we can test every combination of options, should we?

@lcreid
Copy link
Copy Markdown
Contributor Author

lcreid commented Feb 17, 2018

The tests aren't "real-world" in that the form layout is declared one way, and then all the fields inside are declared with another layout. However, our documentation says you can override the form level layout with field level layout, so I think it should be tested.

The test output that should look ugly in Bootply are where the form is layout: :inline and the fields are not. If you saw something unacceptable in any of the other combinations, please let me know.

Thanks for the the merges today, @mattbrictson ! I think we're getting close to alpha.

@lcreid
Copy link
Copy Markdown
Contributor Author

lcreid commented Feb 21, 2018

Doh! It took a while but I think I realized what you were getting at, @mattbrictson . I removed the tests that were testing the overrides of in-line output, so that in the future no one will think that, because it's in a test case, it's the correct output.

Since the links may not be anywhere else, here's what trying to override in-line layouts looks like at the time of writing: https://www.bootply.com/1hMYpBHds5 and: https://www.bootply.com/JqdpfgA36j.

Hopefully this can be approved now. :-)

Copy link
Copy Markdown
Contributor

@mattbrictson mattbrictson left a comment

Choose a reason for hiding this comment

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

Looks good; thanks for the follow up. I am seeing some odd things in the diff, like a repetition of the changes that were already in #438. I'll rebase to hopefully clear that up and then merge.

@mattbrictson
Copy link
Copy Markdown
Contributor

Rebased

@mattbrictson mattbrictson merged commit 19c7785 into bootstrap-ruby:master Feb 23, 2018
@lcreid lcreid deleted the 426-layouts branch February 25, 2018 19:30
lcreid added a commit to lcreid/rails-bootstrap-forms that referenced this pull request Jun 4, 2018
…by#433)

* Test the combinations of layout at form and control level.

* Strip layout from options for check boxes and radios.

* Improve the tests.

* Update tests to new markup.

* All but custom.
Need to check/update all the Bootply rendered versions.

* Tweak label classes for visual rendering.

* Test the combinations of layout at form and control level.

* Strip layout from options for check boxes and radios.

* Improve the tests.

* All but custom.
Need to check/update all the Bootply rendered versions.

* Tweak label classes for visual rendering.

* Remove incorrect tests.

* Check rendering and add links to Bootply.
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.

3 participants