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

amp-date-countdown: migrated getAttributes from constructor to buildCallback #17656

Merged
merged 5 commits into from Sep 6, 2018

Conversation

aghassemi
Copy link
Contributor

Clone of #17622 by @leonalicious

Fixes #17596

amp-date-countdown will have issue of getting the attributes from DOM. We should have written
list of elements which are responsible of getting attributes from DOM, such as this.element.getAttribute('end-date') in buildCallback() instead of constructor()

image

@aghassemi
Copy link
Contributor Author

/cc @leonalicious

@leonahliang90
Copy link
Contributor

@aghassemi @cathyxz may i know when will this PR get merged into master branch please ?

@leonahliang90
Copy link
Contributor

@aghassemi @cathyxz may i know when will this PR get merged into master branch please ?
/cc @choumx

@aghassemi
Copy link
Contributor Author

@leonalicious apologies, ran into some lint issue then was our of office. Will merge it today.

@aghassemi
Copy link
Contributor Author

@leonalicious there were some type issues with the PR which I fixed but now unit tests are failing as they are not compatible with the move from constructor to buildCallback, Could I leave it to you to fix and send another PR?

@aghassemi aghassemi merged commit de4ab2b into ampproject:master Sep 6, 2018
@aghassemi
Copy link
Contributor Author

Never mind, managed to find a fix for it. Merged. Sorry about the delay

Enriqe pushed a commit to Enriqe/amphtml that referenced this pull request Nov 28, 2018
…allback (ampproject#17656)

* amp-date-countdown: migrated getAttributes from constructor to buildCallback

* lint

* define the types in constructor

* set end-date in tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants