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

fix(amp-user-notification): execute GET instead of POST on data-show-if-href #1321

Merged
merged 1 commit into from
Jan 11, 2016

Conversation

erwinmombay
Copy link
Member

closes #1228

@cramforce
Copy link
Member

Nit: This isn't really a refactor. More like a breaking change :)

@erwinmombay
Copy link
Member Author

@cramforce lol good point 🎯

@erwinmombay erwinmombay changed the title [WIP]refactor(amp-user-notification): execute GET instead of POST on data-show-if-href [WIP]fix(amp-user-notification): execute GET instead of POST on data-show-if-href Jan 6, 2016
@erwinmombay erwinmombay force-pushed the notify-get branch 3 times, most recently from 7ced1e6 to 93b0b44 Compare January 6, 2016 23:21
@erwinmombay erwinmombay changed the title [WIP]fix(amp-user-notification): execute GET instead of POST on data-show-if-href fix(amp-user-notification): execute GET instead of POST on data-show-if-href Jan 7, 2016
@erwinmombay
Copy link
Member Author

@dvoytenko ready for review

@erwinmombay
Copy link
Member Author

@dvoytenko PTAL

@@ -205,7 +205,7 @@
<amp-user-notification
layout=nodisplay
id="amp-user-notification1"
data-show-if-href="http://localhost:8000/api/show"
data-show-if-href="http://localhost:8000/api/show?timestamp=$TIMESTAMP"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove $ - works without and we are trying to not advertise them.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

@erwinmombay erwinmombay force-pushed the notify-get branch 2 times, most recently from 3750735 to 3951765 Compare January 8, 2016 11:01
describe('buildGetHref_', () => {

it('should do url replacement', () => {
dftAttrs['data-show-if-href'] = 'https://www.ampproject.org/path/?ord=$RANDOM';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto $

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed. done.

@dvoytenko
Copy link
Contributor

Cool. Couple more comments...

@erwinmombay erwinmombay force-pushed the notify-get branch 2 times, most recently from 081cc31 to 0f347d6 Compare January 11, 2016 10:01
@dvoytenko
Copy link
Contributor

LGTM

erwinmombay added a commit that referenced this pull request Jan 11, 2016
fix(amp-user-notification): execute GET instead of POST on data-show-if-href
@erwinmombay erwinmombay merged commit a5791f1 into ampproject:master Jan 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should the amp-user-notification component do a GET or POST request to the data-show-if-href url
3 participants