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

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

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

Conversation

reflexxion
Copy link

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

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

Dominik added 3 commits August 12, 2013 07:58
…selected)

Also jqMigrate raises an warning on this.
When coming from an earlier jquery-selectBox version, the inline styles overwrites existing css rules
@marcj
Copy link
Owner

marcj commented Aug 12, 2013

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?

@reflexxion
Copy link
Author

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.

@ShawnCG
Copy link
Contributor

ShawnCG commented Aug 13, 2013

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 🐒

@reflexxion
Copy link
Author

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.

@marcj
Copy link
Owner

marcj commented Aug 13, 2013

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.

@reflexxion
Copy link
Author

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" notifications@github.com:

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//pull/132#issuecomment-22597324
.

@ShawnCG
Copy link
Contributor

ShawnCG commented Aug 13, 2013

(4773a31)

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

(715a346)

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 🐵

@reflexxion
Copy link
Author

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

@marcj
Copy link
Owner

marcj commented Aug 14, 2013

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.

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.

None yet

3 participants