amp-date-countdown component #15453

leonahliang90 · 2018-05-21T09:11:04Z

(previously mostly reviewed via PR #13657)

MVP:

Ability to set an enddate (probably ISO format with optional timezone section similar to amp-timeago) and get some default out of the box rendering. (e.g. 2 days 10 hours 2 minutes 30 seconds ) - Done
Ability to customize and internationalize the format: (e.g. 3 hours 10:02:30) - Done
- Since numbers are dynamic, pluralization of the words matter here - Ability to introduce multiple language, I am not language expert so i presume we would consistently use plural instead as different language for different rules and will make the component complicated and heavy.
Ability to provide formats where groupings are different. For instance instead of 1 month 20 days publisher may prefer 50 days - Done, please refer to biggest-unit attribute
Ability to use CSS to style each part (number and text) so publisher could do renderings - Done
Ability to write CSS that would allow animation when numbers change (this probably means we either need to reset the class or remove/reinsert instead of just changing innerText) - Suggest to move to non-MVP list, as there are many css animation effects, it might take quite some time and effort to be implemented
Ability to specify what happens when it reaches the end. Maybe publisher wants it to stay at 0 or they may it to keep going in the opposite direction. Done
One of end-date, timestamp-ms, timestamp-seconds is required, else error message. -
Done
Support offset-seconds - Done
Support i18n locale - Done

To be confirmed：

currently only support up to Day, not sure need to support for Weeks / Months / Years ?
css animation

…ountdown.md

ghost

Docs LGTM

twifkak · 2018-05-29T15:36:14Z

extensions/amp-date-countdown/validator-amp-date-countdown.protoascii

+  }
+  attrs: {
+    name: "offset-seconds"
+    value_regex: "-?\\d{1,}"


Optionally, you could shorten {1,} to +.

twifkak · 2018-05-29T15:37:29Z

Validator changes look good, thanks! (Small regex suggestion in the protoascii.)

aghassemi · 2018-05-31T18:38:02Z

@leonalicious This is getting very close, I see couple of unit tests failures (related to this component). We can merge as soon as they are fixed and build is green. Thanks for all the work on this component!

leonahliang90 · 2018-06-04T06:15:33Z

it was working previously, and i have tried to fix this for last couple of days, seems like the connection between element.setAttribute , build(), impl are not connected and I am lost ...

For instance, based on my understanding

element.setAttribute('biggest-unit', 'HOURS')
then element.build()
then my element.implementation_ should be able to get the object that have attribute('biggest-unit') and execute the method that i want

it was working previously, now is not working, if there are any simple example would be much appreciate

leonahliang90 · 2018-06-05T12:46:34Z

@aghassemi please help, i have uploaded my test-amp-date-countdown.js, the last 3 use cases that i have commented out are not working, i am writing it based on test-amp-date.js but not too sure why they are not working as expected

aghassemi · 2018-06-05T20:55:47Z

extensions/amp-date-countdown/0.1/test/test-amp-date-countdown.js

+  //   element.build();
+  //   let timeObj;
+  //   let itemElement;
+  //   Services.viewerForDoc(win.document).whenFirstVisible().then(() => {


because this test is async you need to add a return here and return the promise so that the test runner knows to wait.

i did add a return here, but then it will stuck there and return error

Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.

the thing is it will never go inside the function, not sure why , and then i did try another approach in the last 2 test cases

looks like you are stubbing the whenFirstVisible above in test setup and keeping a reference under viewer variable so

return viewer.whenFirstVisible().then() { }); whenFirstVisiblePromiseResolve();

should work.

nope, still not working as expected

feel free to skip for now to get the PR in, we can fix later.

Okay, already commented them out , and the rest of the test scripts are passed !

@leonalicious there seems to be a conflict in gulp.js after that's resolved and build is green, we can merge. Thanks for all the work

here you go, should be resolved by now~

a new unit test failure (32:7 error 'whenFirstVisiblePromiseResolve' is assigned a value but never used. Allowed unused vars must match /AmpElement|Def|Interface$/ no-unused-vars
)

(we are constantly improving our lint/jsdoc rules so that is why these issues keep coming up, so as you rebase and try to merge PR, there might be new rules to adhere to that did not exist before. Apologies for the inconvenience this causes but needed for code quality improvements)

i do run sudo gulp pr-check and notice there are too many errors which flood my entire command screen and i couldn't not pin-point and check on amp-date-countdown properly. but i guess all linter check should be achieved by now~

aghassemi · 2018-06-19T16:27:31Z

@leonalicious Merged! Thanks so much for contributing this new component to AMP and for you patience through the review and approval process.
/cc @ericlindley-g

leonahliang90 · 2018-06-20T11:06:19Z

It is our pleasure to contribute the component and giving it back to the community ~

* cl/201378460 Revision bump for #15453 * cl/201393002 Revision bump for #14521 * cl/201426296 Revision bump for #16159 * cl/201570498 n/a * cl/201578514 n/a * cl/201602467 Revision bump for #16167

leonahliang90 and others added 30 commits February 26, 2018 14:36

fix #10385 [amp-date-countdown] contribute amp-date-countdown component

dd004a1

submit, solve jslint presubmit and lint validations, added amp-date-c…

f6e7603

…ountdown.md

updated amp-date-countdown.md and also some naming

277fda5

tiny update

e19cda2

updated test-scripts and tiny bugs for offset-seconds unit

9e8fce9

added timeout action when timer hits 0

902d413

fixed to 2018, setInterval and clearInterval, fat arrow function

02c72bf

remove split and change locale to array, remove extra variable

a854b3d

fixed some tiny issue

da248e2

fix based on suggestions given by carlos

4a9b1e6

fixed based on carlos and aghassemi suggestions - 20180307

dd4ae8e

touchups

d342e5d

touchup2

a2841eb

touchup3

d4ee146

fix 20180312

a42b235

touchups4

deb4da1

change actionTrust from to LOW, change from hide and show to animation

b480973

fix conflict of GULP and also UPDATE ampdemo to lastest amphtml

81fdca8

fix #10385 [amp-date-countdown] contribute amp-date-countdown component

207b061

submit, solve jslint presubmit and lint validations, added amp-date-c…

0b16ef8

…ountdown.md

updated amp-date-countdown.md and also some naming

14aef4e

tiny update

09c97ee

updated test-scripts and tiny bugs for offset-seconds unit

d437130

added timeout action when timer hits 0

7fe99b7

fixed to 2018, setInterval and clearInterval, fat arrow function

bdb60c7

remove split and change locale to array, remove extra variable

8715131

fixed some tiny issue

fb8f95d

fix based on suggestions given by carlos

8058a0e

fixed based on carlos and aghassemi suggestions - 20180307

66f395c

touchups

26b91cd

ghost approved these changes May 28, 2018

View reviewed changes

twifkak reviewed May 29, 2018

View reviewed changes

twifkak approved these changes May 29, 2018

View reviewed changes

small fix on protoascii

fed1519

aghassemi self-assigned this May 30, 2018

leonahliang90 added 2 commits June 5, 2018 14:04

Merge branch 'master' of github.com:ampproject/amphtml

15d08d3

last 3 failed use cases

7aefbe6

aghassemi reviewed Jun 5, 2018

View reviewed changes

leonahliang90 and others added 8 commits June 12, 2018 11:11

fix gulpfile conflict

259d451

fix gulp

509ce72

error reproduce !

00f0910

submit

3089176

fix test-amp-date-countdown

b52e63b

jsdocs

c1480b6

no-mixed-operators fix

a83f146

Merge branch 'master' into master

5995b13

aghassemi changed the title ~~fix #10385 [amp-date-countdown] contribute amp-date-countdown component - ref PR 13657~~ 🌟 amp-date-countdown component Jun 19, 2018

aghassemi changed the title ~~🌟 amp-date-countdown component~~ amp-date-countdown component Jun 19, 2018

aghassemi merged commit 304e09a into ampproject:master Jun 19, 2018

This was referenced Jun 19, 2018

Create amp-date-countdown component #10385

Closed

Add an AMP component that shows a count up/down animation #12177

Closed

alin04 pushed a commit to alin04/amphtml that referenced this pull request Jun 22, 2018

cl/201378460 Revision bump for ampproject#15453

8b14c25

alin04 mentioned this pull request Jun 22, 2018

Validator rollup #16266

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

amp-date-countdown component #15453

amp-date-countdown component #15453

leonahliang90 commented May 21, 2018 •

edited by aghassemi

ghost left a comment

twifkak May 29, 2018

twifkak commented May 29, 2018

aghassemi commented May 31, 2018

leonahliang90 commented Jun 4, 2018

leonahliang90 commented Jun 5, 2018

aghassemi Jun 5, 2018

leonahliang90 Jun 6, 2018

aghassemi Jun 7, 2018

leonahliang90 Jun 9, 2018

aghassemi Jun 12, 2018

leonahliang90 Jun 12, 2018 •

edited

aghassemi Jun 12, 2018

leonahliang90 Jun 12, 2018

aghassemi Jun 12, 2018

leonahliang90 Jun 12, 2018

aghassemi commented Jun 19, 2018

leonahliang90 commented Jun 20, 2018

amp-date-countdown component #15453

amp-date-countdown component #15453

Conversation

leonahliang90 commented May 21, 2018 • edited by aghassemi

MVP:

To be confirmed：

ghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twifkak commented May 29, 2018

aghassemi commented May 31, 2018

leonahliang90 commented Jun 4, 2018

leonahliang90 commented Jun 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leonahliang90 Jun 12, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aghassemi commented Jun 19, 2018

leonahliang90 commented Jun 20, 2018

leonahliang90 commented May 21, 2018 •

edited by aghassemi

leonahliang90 Jun 12, 2018 •

edited