-
Notifications
You must be signed in to change notification settings - Fork 20.5k
Fixes #12957 - Improve wrapMap #1044
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
Conversation
Ticket? Why are you removing a test? |
Ticket? I have one, i created it after i pulled, so i can link ticket to the pull Why are you removing a test? Checkout pull description |
Yes, I saw that. Generally, a ticket comes first, where the issue itself can be discussed. |
The test passes after a re-test btw... Wish there was some way we could get the "recount" to update status on jenkins/mergeatron. |
@gnarf37 file an issue :). We'll be improving mergeatron based on jQuery's usage. |
@jquerybot retest |
src/manipulation.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of this change? It breaks the leading-text case: http://jsfiddle.net/NzCTZ/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gibson042 thanks, i missed that :-(, i added test for this case
It's interesting that this seems to have such broad support (though I would like to confirm on some mobile platforms), but if the closing tags are truly unnecessary for our purposes then I'm fine with dropping them. |
I would like to confirm on some mobile platforms Would you do it? Or you asking me to do it? |
I'll throw some BrowserStack VMs at it, and recommend that you test it too if you can. @toddparker, @uGoMobi, @scottjehl: is there a better way? |
Well, I didn't see any failures introduced by this change. Unless the peanut gallery find a problem, I think it's all good. |
recommend that you test it too if you can @gibson042 Right know i don't have many emulators, although i checked in some of them. |
Tested in BrowserStack VMs as well, LGTM 👍 |
I'm still getting a "Merge With Caution" message on the PR (but seems irrelevant because merges just fine) |
@rwldrn, please ignore the caution for now. |
ignoring |
...how can I let them know that as a result of the unfreezing process, I have no inner monologue? |
I couldn't believe this actually seems to work! |
In most cases end tags are unnecessary and their absence make
jQuery.buildFragment
even more consistent across browsers, whenjQuery.clean
takes incorrect HTML argument as a string.For example – in this pull, specifically in this line,
jQuery#html
takes as argument incorrect HTML-string, in new browsers,area
element will be created, but in oldIE, you will have onlymap
element, btw, oldIE always returncoords
value, even if it is not defined.Incorrect HTML-strings should not be passed to jQuery methods, but it's nice when jQuery can process them more consistently across browsers and it will shave off couple of bytes.