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

adding method of unifying current url and alias #10146

Conversation

wronan
Copy link
Contributor

@wronan wronan commented May 12, 2016

adding method of unifying current url and alias so both start (or don't start) with '/'

Original ticket: #9872

@tsteur
Copy link
Member

tsteur commented May 12, 2016

Can you build a simple html page with a link and the tracking code to reproduce this issue?

Otherwise we can't test it and check for regressions. Also let us know about operating system and exact browser version. We did some tests with IE11 recently and it worked fine but it's possible that it only occurs in a certain case.

@tsteur tsteur added the Needs Review PRs that need a code review label May 12, 2016
@wronan
Copy link
Contributor Author

wronan commented May 12, 2016

Hi @tsteur
@mgonera wrote that problem occurs on IE11, then he changed his mind (or Internet Explorer changed his mind - IE has a mind of it's own :D ) because problem is with IE10 (but the title stayed as was).

I'm cooperating with @mgonera, we have a test site (but it's simple - very simple or even Very simple one may say). Don't want to post the links here, because he was the one to setup it, it's on his server. He will probably share it without a problems but I don't want to do this for him. @mgonera, would you be so kind?

@mgonera
Copy link

mgonera commented May 12, 2016

@wronan share whatever is needed, I think they already are here somewhere. Just pass the credentials privately and please indicate the directories on the server (as you may remember I gave you access one level higher than I was supposed to). thanks.

@tsteur
Copy link
Member

tsteur commented May 13, 2016

A simple HTML page could be enough eg with a "paste" service. We don't need a URL

@mgonera
Copy link

mgonera commented May 16, 2016

@tsteur I provided this to your e-mail directly

@mattab mattab added this to the 2.16.x (LTS) milestone May 27, 2016
@mgonera
Copy link

mgonera commented Jun 9, 2016

@tsteur can you review it?

@tsteur
Copy link
Member

tsteur commented Jun 9, 2016

My time is a bit limited these days but I can already tell that tests are missing. From the looks it seems like a valid patch but it'll take some time to test it etc. When there are tests then it'll be easier to later to review

@mattab
Copy link
Member

mattab commented Jul 7, 2016

Review

  • please add an automated tests in tests/javascripts/index.php

@mattab
Copy link
Member

mattab commented Jul 14, 2016

Thanks for the PR @wronan! I created this new PR with the tests: #10302 and changed the code slightly. we'll merge it in 2.16.2 👍

@mattab mattab closed this Jul 14, 2016
@wronan
Copy link
Contributor Author

wronan commented Jul 14, 2016

Thx man. Sorry for the delay with tests. I'm overwhelmed with work... Till the next PR then :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants