Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Issue 6810 #6811

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
5 participants

These changes should fix the issue.

Member

gseguin commented Dec 18, 2013

@mnaughto,

Thank you for your contribution! It looks like you haven't signed our CLA. Could you do so at http://contribute.jquery.org/CLA/ Thank you in advance.

It should be signed now.

On Dec 18, 2013, at 15:46, Ghislain Seguin notifications@github.com wrote:

@mnaughto,

Thank you for your contribution! It looks like you haven't signed our CLA. Could you do so at http://contribute.jquery.org/CLA/ Thank you in advance.


Reply to this email directly or view it on GitHub.

fjsj commented Dec 19, 2013

This pull-request also fixes #6746

@arschmitz arschmitz commented on the diff Jan 1, 2014

js/jquery.mobile.support.js
@@ -111,7 +111,7 @@ function transform3dTest() {
// Test for dynamic-updating base tag support ( allows us to avoid href,src attr rewriting )
function baseTagTest() {
- var fauxBase = location.protocol + "//" + location.host + location.pathname + "ui-dir/",
+ var fauxBase = location.protocol + ((location.host !== "") ? "//" : "") + location.host + location.pathname + "ui-dir/",
@arschmitz

arschmitz Jan 1, 2014

Owner

should be spaces inside each set of parentheses

@arschmitz arschmitz commented on the diff Jan 1, 2014

js/navigation/path.js
@@ -53,7 +53,7 @@ define([
// Make sure to parse the url or the location object for the hash because using location.hash
// is autodecoded in firefox, the rest of the url should be from the object (location unless
// we're testing) to avoid the inclusion of the authority
- return uri.protocol + "//" + uri.host + uri.pathname + uri.search + hash;
+ return uri.protocol + ((uri.host !== "") ? "//" : "") + uri.host + uri.pathname + uri.search + hash;
@arschmitz

arschmitz Jan 1, 2014

Owner

missing spaces here as well

@arschmitz arschmitz commented on the diff Jan 1, 2014

js/navigation/path.js
@@ -323,7 +323,7 @@ define([
// reconstruct each of the pieces with the new search string and hash
href = path.parseUrl( href );
- href = href.protocol + "//" + href.host + href.pathname + search + preservedHash;
+ href = href.protocol + ((href.host !== "") ? "//" : "") + href.host + href.pathname + search + preservedHash;
} else {
@arschmitz

arschmitz Jan 1, 2014

Owner

spacing here also

Owner

arschmitz commented Jan 1, 2014

In addition to the above comments can you please see http://contribute.jquery.org/commits-and-pull-requests/#commit-guidelines for proper formatting of your commit messages. and if you have any questions about the spacing or our style guide in general you can see http://contribute.jquery.org/style-guide/js/ for our js style guide

One last thing is this also needs a test added to ensure no future regressions with this issue.

once you update the PR please comment and i will look again ( i don't get a notification you updated if you don't comment )

Owner

arschmitz commented Feb 6, 2014

@mnaughto do you plan on updating this?

@gabrielschulhof gabrielschulhof self-assigned this Mar 11, 2014

gabrielschulhof added a commit that referenced this pull request Mar 11, 2014

Contributor

gabrielschulhof commented Mar 11, 2014

I've made the requested modifications in #7237.

gabrielschulhof added a commit that referenced this pull request Mar 14, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment