jquery-selectBox icon indicating copy to clipboard operation
jquery-selectBox copied to clipboard

Use jQuery's .prop() and add disableAutoStyle option

Open reflexxion opened this issue 12 years ago • 9 comments

As of jQuery 1.6 element properties should be accessed with jQuery's .prop() method (715a34623390fdc706968e4666d060e29728cd4f)

When coming from an earlier jQuery-selectBox version (I came from 1.0.6) setting inline styles overwrites existing css definitions. (4773a31efb48bac537e40f3df1174b7fbbc9cc1f)

reflexxion avatar Aug 12 '13 06:08 reflexxion

It seems you've not configured your git username/email correctly in your environment? Btw, why not just setting your styles via xx !important in the css?

marcj avatar Aug 12 '13 22:08 marcj

Ahh, right - haven't added the other email to my account. Thanks! (As you probably saw, first real GitHub commit)

If I can avoid using !important as easy as with this patch I do it.

reflexxion avatar Aug 13 '13 05:08 reflexxion

According to your code

control
.width(settings.disableAutoStyle ? '' : select.outerWidth())
.addClass(select.attr('class'))
.attr('title', select.attr('title') || '')
.attr('tabindex', tabIndex)
.css('display', settings.disableAutoStyle ? '' : 'inline-block')

if settings.disableAutoStyle is set to true wouldn't that actually clear any styles you have set for display or width

http://jsfiddle.net/aPFS9/

Correct me if I'm wrong :monkey:

ShawnCG avatar Aug 13 '13 13:08 ShawnCG

No, you are right.

But control is a generated element - there shouldn't be any styles at this stage. And it would only unset inline-styles.

reflexxion avatar Aug 13 '13 13:08 reflexxion

So, the new option is only about the label element? The width or display property? I believe the explicit setting of a width is anyway useless, so we should probably just remove this.width() call - inline-block does already the correct sizing.

marcj avatar Aug 13 '13 21:08 marcj

Why set styles at all? There is a stylesheet delivered in the package. Shouldn't that be enough? Am 13.08.2013 23:03 schrieb "Marc J. Schmidt" [email protected]:

So, the new option is only about the label element? The width or display property? I believe the explicit setting of a width is anyway useless, so we should probably just remove this.width() call - inline-block does already the correct sizing.

— Reply to this email directly or view it on GitHubhttps://github.com/marcj/jquery-selectBox/pull/132#issuecomment-22597324 .

reflexxion avatar Aug 13 '13 21:08 reflexxion

(4773a31efb48bac537e40f3df1174b7fbbc9cc1f)

I wouldn't remove styles as there might be people who also depend on these functions of the plugin But if we did we should at least keep setting the display

(715a34623390fdc706968e4666d060e29728cd4f)

As for the .prop()

_"The most common boolean attributes are checked, selected, disabled, and readOnly, but here is a full list of boolean attributes/properties that jQuery 1.6.1 supports dynamically getting and setting with .attr():_

autofocus, autoplay, async, checked, controls, defer, disabled,
hidden, loop, multiple, open, readonly, required, scoped, selected

_It is still recommended that .prop() be used when setting these boolean attributes/properties"_ jQuery 1.6.1 rc 1 released

Just putting that there for reference :monkey_face:

ShawnCG avatar Aug 13 '13 21:08 ShawnCG

Yeah that's right, so the setting is a disable one, so that by default it wouldn't change anything for upgrading users.

reflexxion avatar Aug 14 '13 06:08 reflexxion

I don't think that's a big deal. Setting the styles through the css file instead of inline-css should make it the same to upgrading users.

marcj avatar Aug 14 '13 11:08 marcj