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

EOY 2015 Preserve existing url params #61

Merged
merged 1 commit into from Nov 16, 2015

Conversation

Projects
None yet
2 participants
@ScottDowne
Collaborator

ScottDowne commented Nov 11, 2015

… prop

@ScottDowne

This comment has been minimized.

Show comment
Hide comment
@ScottDowne

ScottDowne Nov 11, 2015

Collaborator

This allows for the config prop donationFormURL to already have query string params pre filled, and the snippet knows to handle that, and if it doesn't the snippet still knows.

It also makes sure we default to the same preset values to match the snippet even if frequency is changed. Example, if you change frequency to monthly, the amounts preselected match the snippet at 20,10,5,3, but if you change it to a single donation, it was defaulting it to the wrong amounts.

@glogiotatidis quick review?

Collaborator

ScottDowne commented Nov 11, 2015

This allows for the config prop donationFormURL to already have query string params pre filled, and the snippet knows to handle that, and if it doesn't the snippet still knows.

It also makes sure we default to the same preset values to match the snippet even if frequency is changed. Example, if you change frequency to monthly, the amounts preselected match the snippet at 20,10,5,3, but if you change it to a single donation, it was defaulting it to the wrong amounts.

@glogiotatidis quick review?

@glogiotatidis

This comment has been minimized.

Show comment
Hide comment
@glogiotatidis

glogiotatidis Nov 12, 2015

Member

I noticed that you set target=_blank here https://github.com/ScottDowne/snippets/blob/65517b7d8cddf360c8ffcb4ffb8caa4af98419af/campaigns/mofo-eoy-2015/snippet.html#L228 which is blocked by default in fx and the "the page wants to open a popup" top bar appears. Let's change that.

Member

glogiotatidis commented Nov 12, 2015

I noticed that you set target=_blank here https://github.com/ScottDowne/snippets/blob/65517b7d8cddf360c8ffcb4ffb8caa4af98419af/campaigns/mofo-eoy-2015/snippet.html#L228 which is blocked by default in fx and the "the page wants to open a popup" top bar appears. Let's change that.

@glogiotatidis

This comment has been minimized.

Show comment
Hide comment
@glogiotatidis

glogiotatidis Nov 12, 2015

Member

Please set a proper capitalized commit message.

Member

glogiotatidis commented Nov 12, 2015

Please set a proper capitalized commit message.

@ScottDowne

This comment has been minimized.

Show comment
Hide comment
@ScottDowne

ScottDowne Nov 12, 2015

Collaborator

Thanks for the review, working on these now.

Collaborator

ScottDowne commented Nov 12, 2015

Thanks for the review, working on these now.

@ScottDowne

This comment has been minimized.

Show comment
Hide comment
@ScottDowne

ScottDowne Nov 12, 2015

Collaborator

@glogiotatidis Fixed :)

Collaborator

ScottDowne commented Nov 12, 2015

@glogiotatidis Fixed :)

@glogiotatidis

This comment has been minimized.

Show comment
Hide comment
@glogiotatidis

glogiotatidis Nov 13, 2015

Member

re commit message I think something along the lines EOY 2015 Preserve existing url params is better :)

Member

glogiotatidis commented Nov 13, 2015

re commit message I think something along the lines EOY 2015 Preserve existing url params is better :)

@glogiotatidis

This comment has been minimized.

Show comment
Hide comment
@glogiotatidis

glogiotatidis Nov 13, 2015

Member

This is good to go, lets bring back snippet_name and I'll merge and put to prod. Thanks @ScottDowne 🐙

Member

glogiotatidis commented Nov 13, 2015

This is good to go, lets bring back snippet_name and I'll merge and put to prod. Thanks @ScottDowne 🐙

@ScottDowne

This comment has been minimized.

Show comment
Hide comment
@ScottDowne

ScottDowne Nov 13, 2015

Collaborator

Updated.

One question, I needed to add |safe to the {url} bit, so the & in the url doesn't get converted to &

But I noticed the params are added as "https://donate.mozilla.org/de/?utm_source=desktop-snippet&utm_medium=snippet&utm_term=5465&utm_campaign=EOY" with a & already in it. Does that make sense and is this going to come end up how we want it once it's shipped?

Collaborator

ScottDowne commented Nov 13, 2015

Updated.

One question, I needed to add |safe to the {url} bit, so the & in the url doesn't get converted to &

But I noticed the params are added as "https://donate.mozilla.org/de/?utm_source=desktop-snippet&utm_medium=snippet&utm_term=5465&utm_campaign=EOY" with a & already in it. Does that make sense and is this going to come end up how we want it once it's shipped?

@glogiotatidis glogiotatidis changed the title from fix 2015 eoy snippet to allow for existing query string params in url… to EOY 2015 Preserve existing url params Nov 16, 2015

@glogiotatidis

This comment has been minimized.

Show comment
Hide comment
@glogiotatidis

glogiotatidis Nov 16, 2015

Member

But I noticed the params are added as "https://donate.mozilla.org/de/?utm_source=desktop-snippet&utm_medium=snippet&utm_term=5465&utm_campaign=EOY" with a & already in it. Does that make sense and is this going to come end up how we want it once it's shipped?

Good question. When you say params are added you mean that the snippet editor pastes a link with & already in it, correct?

Member

glogiotatidis commented Nov 16, 2015

But I noticed the params are added as "https://donate.mozilla.org/de/?utm_source=desktop-snippet&utm_medium=snippet&utm_term=5465&utm_campaign=EOY" with a & already in it. Does that make sense and is this going to come end up how we want it once it's shipped?

Good question. When you say params are added you mean that the snippet editor pastes a link with & already in it, correct?

@glogiotatidis

This comment has been minimized.

Show comment
Hide comment
@glogiotatidis

glogiotatidis Nov 16, 2015

Member

Does that make sense and is this going to come end up how we want it once it's shipped?

I just verified this. It will come up with & but that's ok since that's the way to do it in xhtml and about:home is xhtml

Member

glogiotatidis commented Nov 16, 2015

Does that make sense and is this going to come end up how we want it once it's shipped?

I just verified this. It will come up with & but that's ok since that's the way to do it in xhtml and about:home is xhtml

@@ -225,7 +225,7 @@
{{ donationAmountFourth }} {{ currencySymbol|default("$", true) }}
</button>
<a class="button button-red button-wide donate-button {{ buttonAnimate|default("", true) }}" target="_blank" href="#">
<a class="button button-red button-wide donate-button {{ buttonAnimate|default("", true) }}" href="#">

This comment has been minimized.

@glogiotatidis

glogiotatidis Nov 16, 2015

Member

btw we are now expecting templates to be valid XML so this would raise an error while saving. I changed it to {% if buttonAnimate %} {{ buttonAnimate }} {% endif %}

@glogiotatidis

glogiotatidis Nov 16, 2015

Member

btw we are now expecting templates to be valid XML so this would raise an error while saving. I changed it to {% if buttonAnimate %} {{ buttonAnimate }} {% endif %}

This comment has been minimized.

@ScottDowne

ScottDowne Nov 16, 2015

Collaborator

Works for me :)

@ScottDowne

ScottDowne Nov 16, 2015

Collaborator

Works for me :)

@glogiotatidis

This comment has been minimized.

Show comment
Hide comment
@glogiotatidis

glogiotatidis Nov 16, 2015

Member

Thanks!

Member

glogiotatidis commented Nov 16, 2015

Thanks!

glogiotatidis added a commit that referenced this pull request Nov 16, 2015

Merge pull request #61 from ScottDowne/url-fixes
 EOY 2015 Preserve existing url params

@glogiotatidis glogiotatidis merged commit d117559 into mozmeao:master Nov 16, 2015

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