Skip to content
This repository has been archived by the owner on Jan 24, 2020. It is now read-only.

EOY 2015 anchor to work with & in the href #64

Merged
merged 1 commit into from Nov 17, 2015

Conversation

ScottDowne
Copy link
Contributor

No description provided.

@ScottDowne
Copy link
Contributor Author

@glogiotatidis We noticed something, the url needs the & to be parsed and replaced with &, as seen in last year's snippet: https://github.com/mozilla/snippets/blob/master/campaigns/mofo-eoy-2014/deliverables/eoy2-deliverable.html#L246

review?

@glogiotatidis
Copy link
Contributor

@schalkneethling can you shed some light here? As you know about:home is xhtml and we can a link in this template that looks like this

https://donate.mozilla.org/de/?utm_source=desktop-snippet&utm_medium=snippet&utm_term=5465&utm_campaign=EOY15

I understand that & is the correct way to put & in a xhtml page and it should reach donate.mozilla.org in the correct form. Is that correct? Do we need what Scott is doing? Is it ok to use JS to add invalid xhtml elements in the page?

@schalkneethling
Copy link
Contributor

@glogiotatidis Yes, technically having the ampersands escaped is absolutely fine and the URL should reach it's intended destination just fine. I did run into a problem with these exact URLs where using & caused the parameters to not be interpreted correctly so, perhaps it is safe in this case to go with the JS workaround. Especially if it was needed previously.

@glogiotatidis
Copy link
Contributor

Especially if it was needed previously.

We don't have a history with this workaround. AFAIK all snippet links are &.

Can any or both of you give more info about the problems with these urls? I did test them and I did read the parameters correctly on the server side.

@ScottDowne
Copy link
Contributor Author

When I test it, locally, the brief moment it was prod, and when I am shared the results from staging, the link I was getting redirected to had the & in them, which isn't going to be parsed correctly, or is that intended?

It's interesting that last year's had this workaround, which is why I thought it was needed.

Can I see an example of where this is working?

@glogiotatidis
Copy link
Contributor

It's interesting that last year's had this workaround, which is why I thought it was needed.

aha ok good to know

Can I see an example of where this is working?

I just tried it with this template code and custom bottle app and it worked like a charm

@ScottDowne
Copy link
Contributor Author

@glogiotatidis I'm more than happy to not have my change, if you can you confirm first that the & is in fact just an & after the url redirects?

@ScottDowne
Copy link
Contributor Author

@glogiotatidis also, this brings me to another question. I'm adding in params manually, which I use regular & and not & would that be the same issue? Maybe mixing them is causing the issue?

@glogiotatidis
Copy link
Contributor

@glogiotatidis also, this brings me to another question. I'm adding in params manually, which I use regular & and not & would that be the same issue? Maybe mixing them is causing the issue?

that's an interesting point. do you mind testing this out? Also how do you test?

@glogiotatidis
Copy link
Contributor

@glogiotatidis I'm more than happy to not have my change, but can you confirm first that the & is in fact just an & after the url redirects?

Yes, with what I tried the bottle app parsed correctly the query arguments.

@ScottDowne
Copy link
Contributor Author

@glogiotatidis I'm testing i locally in a node app, and I am aware that it wouldn't be a reliable example of what's happening on staging or production, which is why I didn't initially push very hard about this being broken.

However, the staging links I am shared are retaining the & so I started to consider that it might not just be me.

@ScottDowne
Copy link
Contributor Author

I'm looking at the snippet home page, and the about mozilla link at the top has the & in the source, but when I click it, I get the & as expected. So I think you are on to something, but I'm going to do more poking around to understand it better.

@glogiotatidis
Copy link
Contributor

I'm looking at the snippet home page, and the about mozilla link at the top has the & in the source, but when I click it, I get the & as expected. So I think you are on to something, but I'm going to do more poking around to understand it better.

Please do, I'm interested in your findings.

@ScottDowne
Copy link
Contributor Author

@glogiotatidis

Interesting, so I made the anchor tag:
<a class="button button-red button-wide donate-button " href="https://donate.mozilla.org/de/?testone=test&amp;testtwo=test"> Jetzt spenden </a>

and it returns with a working & in the url.

Once I add it via javascript, it breaks. I think the &amp; parsing happens before javascript. If I view source on the page, I see the &amp; but if I inspect it at run time, it's already converted to a &

@ScottDowne
Copy link
Contributor Author

Based on this, I think seeing how I am only using the url inside javascript, it might be optimal to never have the &amp; in the url in the first place, but for consistency sake, I see value in keeping the &amp; in there and having my special case parse it out.

I could also drop the value right in the element instead of js, and then just append to it. That might be optimal, actually.

@ScottDowne
Copy link
Contributor Author

I think I'm making a new pull request with my findings, solving this mystery.

@ScottDowne
Copy link
Contributor Author

@glogiotatidis fixed! Updating commit message, and I think this fix is going to make us both happy.

@glogiotatidis glogiotatidis changed the title EOY 2015 replace url &amp; with & EOY 2015 anchor to work with &amp; in the href Nov 17, 2015
@glogiotatidis
Copy link
Contributor

Works just fine. Many thanks for your research and fix Scott.

glogiotatidis added a commit that referenced this pull request Nov 17, 2015
 EOY 2015 anchor to work with &amp; in the href
@glogiotatidis glogiotatidis merged commit ff31d6d into mozmeao:master Nov 17, 2015
@ScottDowne
Copy link
Contributor Author

@glogiotatidis and thanks for your patience :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants