ember-forge / ember-forge-ui-bootstrap4-polyfill

Provides remaining ember-forge-ui capabilities that ember-forge-ui-bootstrap4 was unable to
MIT License
0 stars 0 forks source link

Should the "form-control" class be added to the input[type="range"] control? #1

Closed notmessenger closed 8 years ago

notmessenger commented 8 years ago

Per the chart at the end of the http://v4-alpha.getbootstrap.com/components/forms/#form-controls section the range control is not one that Bootstrap has a style for or styles by default.

If you use this control along side other ones in the list that have the .form-control class applied to them the range control ends up being much smaller in width than the other controls, which expand to fit the column they are contained within. Applying the .form-control class though causes the range control to behave and appear just like the other (similar) controls.

So in https://github.com/ember-forge/ember-forge-ui-bootstrap4-polyfill/commit/2be9bc6da0c4780aedf4f398bea05efee6a3d91d I added the .form-control class to be applied by default for the range control.

Is this a configuration we wish to present as the default when using the Twitter Bootstrap 4 companion add-on(s) for Ember Forge UI?

notmessenger commented 8 years ago

TWBS has a lint rule that enforces .form-control from not being applied to range controls - https://github.com/twbs/bootlint/wiki/E042#form-control-cannot-be-used-on-non-textual-inputs-such-as-those-whose-type-is-file-checkbox-radio-range-button

notmessenger commented 8 years ago

Perhaps a little more insight https://github.com/twbs/bootstrap/issues/19812

notmessenger commented 8 years ago

https://github.com/twbs/bootstrap/issues/11177#issuecomment-26680270 (and the remaining comments on the page, specifically https://github.com/twbs/bootstrap/issues/11177#issuecomment-28481767)

notmessenger commented 8 years ago

https://github.com/twbs/bootstrap/issues/12746

notmessenger commented 8 years ago

So it seems that TWBS is adamant that .form-control not be used in range controls, though https://github.com/twbs/bootstrap/issues/11177#issuecomment-28481767 points out the advantages of doing so. So I guess that remains the crux of this issue - do we or don't we?

.form-control-range or .w-100 class options came up in these linked issues, with the former seeming to only be a v3 feature possibly, but I need to dig into both of these more to see if they convey the benefits listed in https://github.com/twbs/bootstrap/issues/11177#issuecomment-28481767 but none of the capabilities of .form-control?

notmessenger commented 8 years ago

Me again 😄 Perhaps all of this discussion points out the obvious - this repo should not be applying the .form-control class or any others and let consuming applications make such decisions they feel are best for their (current) usage and needs.

notmessenger commented 8 years ago

One last thought. In keeping with https://github.com/ember-forge/ember-forge-ui-bootstrap4-polyfill/blob/master/addon/components/ef-list-divider.js#L51 which recognizes that Bootstrap has no concept of a list divider but can at least have the same basic classes applied to it as an <li> element perhaps the course of action should be to research the classes mentioned in https://github.com/ember-forge/ember-forge-ui-bootstrap4-polyfill/issues/1#issuecomment-239811971 (not .form-control) and see if applying any of them by default is the way forward.

davewasmer commented 8 years ago

I think the Bootstrap companion addon should only apply standard Bootstrap styles. If the official word from Bootstrap is that .form-control doesn't apply to range controls, then I'd say don't apply it. If a user wants to contravene the Bootstrap standard, they are free to do so in their own implementation.

notmessenger commented 8 years ago

Removing the application of this style