✨ [bento][amp-date-countdown] Initial Preact component for amp-date-countdown #30092

krdwan · 2020-09-02T16:00:59Z

Tracking Issue: #30052
Design Doc: Doc Link

Initial amp-date-countdown preact component. Preact component takes in user defined props to calculate a date to countdown to. This is labeled epoch. Then we use setInterval to continuously calculate the difference between the current time and the epoch time (this is labeled timeLeft).

Please see Design Doc for more details on the component.

Design is similar to DateDisplay Bento component.

krdwan · 2020-09-02T16:04:41Z

src/preact/context.js

@@ -59,3 +59,11 @@ export function WithAmpContext(props) {

  return <AmpContext.Provider children={props['children']} value={current} />;
 }
+
+/**
+ *


Might need some help here with typing.

This is for now a JsonObject. Not for much longer I think, but still the case. I'll ping you when this changes if you haven't yet merged this pull request.

Here it is: #30102

Thanks Dima sounds good!

jridgewell · 2020-09-02T18:44:37Z

extensions/amp-date-countdown/1.0/date-countdown.js

+  if (input < -9 || input > 9) {
+    return String(input);
+  } else if (input >= -9 && input < 0) {
+    return '-0' + Math.abs(input);


Nit:

Suggested change

return '-0' + Math.abs(input);

return '-0' + (-input);

Sounds good updated!

jridgewell · 2020-09-02T20:26:37Z

extensions/amp-date-countdown/1.0/date-countdown.js

+/**
+ * @param {?string} message
+ */
+function throwWarning(message) {


Nit: Let's rename this, since it doesn't actually throw.

Sounds good updated to displayWarning

dvoytenko · 2020-09-02T21:38:38Z

extensions/amp-date-countdown/1.0/date-countdown.js

+
+  useEffect(() => {
+    const interval = setInterval(() => {
+      const newTimeLeft = new Date(epoch) - new Date();


new Date(epoch) is invariant to the interval loop, so let's move it out of setInterval callback. Also if you switch to using Date.now() you can avoid extra allocations.

Thanks, moved new Date(epoch) out of the interval loop and changed epoch variable to be it's Date formatted version.

Also switched to using Date.now()

dvoytenko · 2020-09-02T21:38:54Z

extensions/amp-date-countdown/1.0/date-countdown.js

+  useEffect(() => {
+    const interval = setInterval(() => {
+      const newTimeLeft = new Date(epoch) - new Date();
+      setTimeLeft(() => newTimeLeft);


Why callback setter?

Did not realize the appropriate syntax for this. Have updated.

extensions/amp-date-countdown/1.0/date-countdown.js

dvoytenko · 2020-09-02T21:41:31Z

extensions/amp-date-countdown/1.0/date-countdown.js

+  const [timeLeft, setTimeLeft] = useState(new Date(epoch) - new Date());
+
+  useEffect(() => {
+    const interval = setInterval(() => {


We have to call setInterval and clearInterval from an actual window here. Unlike a one-off setTimeout or open, the risk of leaking from setInterval is very real. So, please add a ref to the root element and use it to resolve window (win = element.ownerDocument.defautlView) and use that window to call set/clearInterval.

Sounds good, updated!

dvoytenko · 2020-09-02T21:42:17Z

extensions/amp-date-countdown/1.0/date-countdown.js

+    return () => clearInterval(interval);
+  }, [playable]);
+
+  const data = Object.assign(


Let's do {...getYDMDMDJMD(), ...getLocaleWord()}.

Sounds good updated!

dvoytenko · 2020-09-02T21:45:19Z

extensions/amp-date-countdown/1.0/date-countdown.js

+ * @return {Object}
+ */
+function getYDHMSFromMs(ms, biggestUnit) {
+  /** @enum {number} */


This is not a enum since this map is accessed by a variable key. This is Object<string, number>

Thanks, updated in the hoisted const.

dvoytenko · 2020-09-02T21:47:00Z

extensions/amp-date-countdown/1.0/date-countdown.js

+ * days, hours, minutes, etc. and returns formatted strings in an object.
+ * @param {number} ms
+ * @param {string} biggestUnit
+ * @return {Object}


This seems like a JsonObject as a result.

Not seeing this change. Did you push?

I think I missed it, it should be included in the latest revision. Thanks!

dvoytenko · 2020-09-02T21:47:57Z

extensions/amp-date-countdown/1.0/date-countdown.type.js

+
+/**
+ * @typedef {{
+ *   endDate (string|undefined),


colons missing

Thanks, updated!

dvoytenko · 2020-09-02T21:49:15Z

src/preact/context.js

@@ -59,3 +59,11 @@ export function WithAmpContext(props) {

  return <AmpContext.Provider children={props['children']} value={current} />;
 }
+
+/**
+ *


This is for now a JsonObject. Not for much longer I think, but still the case. I'll ping you when this changes if you haven't yet merged this pull request.

extensions/amp-date-countdown/1.0/date-countdown.js

krdwan · 2020-09-03T20:18:21Z

Thanks for the review @dvoytenko @jridgewell . New revision is up with changes addressed. Key highlights:

Use useRef to explicitly resolve window
Countdown will not be initiated (or will stop if already initiated) if AMP context -> playable is false
Moved locale strings into a new config file
Use useMemo to remove repetitive calculations on every interval

May have a few more updates / merge conflict resolutions after #30102 is merged.

dvoytenko · 2020-09-03T22:28:04Z

extensions/amp-date-countdown/1.0/date-countdown-config.js

@@ -0,0 +1,52 @@
+/**


Let's rename this file to just messages.js.

Sounds good updated!

dvoytenko · 2020-09-03T22:29:09Z

extensions/amp-date-countdown/1.0/date-countdown-config.js

+ * Strings representing years, minutes, etc. in various locales
+ * @typedef {Array<string>}
+ */
+let DateCountdownLocaleDef;


No need for this type def. We just should use !Array<string> inline.

dvoytenko · 2020-09-03T22:30:17Z

src/preact/context.js

+/**
+ * @return {JsonObject}
+ */
+export function useAmpContext() {


Rebase. This should no longer be needed.

Thanks updated!

dvoytenko · 2020-09-03T22:31:01Z

extensions/amp-date-countdown/1.0/date-countdown.js

+  ...rest
+}) {
+  useResourcesNotify();
+  const {'playable': playable} = useAmpContext();


Should no longer need quotes. Rebase and go back to const {playable} = useAmpContext()

Thanks, updated!

dvoytenko · 2020-09-03T22:33:19Z

extensions/amp-date-countdown/1.0/date-countdown.js

+ * days, hours, minutes, etc. and returns formatted strings in an object.
+ * @param {number} ms
+ * @param {string} biggestUnit
+ * @return {Object}


Not seeing this change. Did you push?

samouri · 2020-09-08T13:56:37Z

extensions/amp-date-countdown/1.0/date-countdown.js

+    }
+    const win = rootRef.current.ownerDocument.defaultView;
+    const interval = win.setInterval(() => {
+      const newTimeLeft = epoch - Date.now() + DELAY;


what is the + DELAY for?

Since the setInterval's callback is not being called until 1000ms (1 delay) from now, setting the correct time to be displayed at that time. So it will start with the initial value being displayed, and calculate the new time to be 1000ms less than the initial value, and will display that in 1000ms.

samouri · 2020-09-08T13:58:30Z

extensions/amp-date-countdown/1.0/date-countdown.js

+  const rootRef = useRef(null);
+
+  useEffect(() => {
+    if (!playable) {


Should we also early-return if the rootRef isn't set yet? e.g.:

if (!playable || ! rootRef.current) { return }

Thanks, updated!

extensions/amp-date-countdown/1.0/date-countdown.js

samouri · 2020-09-08T14:01:33Z

extensions/amp-date-countdown/1.0/date-countdown.js

+    epoch = timestampSeconds * 1000;
+  }
+
+  if (epoch === undefined) {


What is the expected behavior when multiple are provided? e.g. both endDate and timeleftMs

In the current 0.1 implementation, it checks for the first one that is available in the order endDate, timeleftMs, timestampMs, and timestampeSeconds similar to how it's done here.

krdwan · 2020-09-08T20:52:03Z

Thanks for reviews @dvoytenko @samouri !

Latest revision should have all requested changes!

samouri · 2020-09-08T20:53:19Z

extensions/amp-date-countdown/1.0/date-countdown.js

+    () =>
+      new Date(
+        getEpoch(endDate, timeleftMs, timestampMs, timestampSeconds) +
+          offsetSeconds * DELAY


Probably should use MILLISECONDS_IN_SECOND here instead of DELAY right?

Thanks, updated!

extensions/amp-date-countdown/1.0/date-countdown.js

jridgewell · 2020-09-08T22:27:36Z

extensions/amp-date-countdown/1.0/messages.js

+ * Strings representing years, minutes, etc. in various locales
+ * @type {Object<string, !Array<string>>}
+ */
+const LOCALE_WORD = {


Why don't we just store this in the expected { years: … } format? We could then avoid the useMemo and everything else necessary.

So the idea would be to do something like LOCALE_WORD[props['locale']] at each iteration?

One thing I am concerned about is there is a helper function that checks that the props['locale'] value is valid and displays a warning if not. The warning really should only be displayed one time. So even if we were to change the format, we would still have to useMemo to store some info about what our locale is or that the warning message has already been displayed.

Happy to update the format, if there's something I'm missing. If I am misunderstanding please let me know!

jridgewell · 2020-09-08T22:28:39Z

extensions/amp-date-countdown/1.0/date-countdown.js

+    () =>
+      new Date(
+        getEpoch(endDate, timeleftMs, timestampMs, timestampSeconds) +
+          offsetSeconds * DELAY


extensions/amp-date-countdown/1.0/date-countdown.js

dvoytenko

LG on my side. 1 question. Please complete the review with others.

extensions/amp-date-countdown/1.0/date-countdown.js

krdwan · 2020-09-09T15:57:59Z

Thanks for reviews @dvoytenko @jridgewell @samouri !

Latest revision is up with responses to comments!

dvoytenko · 2020-09-10T22:27:04Z

LG from me. But please wait for another LG.

…ountdown (ampproject#30092) * First commit Preact component * Add clear comments for helper functions * Various review comment updates * More review updates * More review updates * Add useMemo * Updates for code review * Review comments and rebase * Remove rebase conflict * remove quotes on playable * Remove date format from epoch * add quotes for object keys that are output to template

google-cla bot added the cla: yes label Sep 2, 2020

krdwan commented Sep 2, 2020

View reviewed changes

krdwan requested review from jridgewell, dvoytenko and alanorozco September 2, 2020 16:15

krdwan marked this pull request as ready for review September 2, 2020 16:16

jridgewell reviewed Sep 2, 2020

View reviewed changes

dvoytenko suggested changes Sep 2, 2020

View reviewed changes

krdwan mentioned this pull request Sep 3, 2020

[amp-date-countdown:1.0] Bento tracking issue #30052

Open

12 tasks

krdwan requested review from dvoytenko and jridgewell September 3, 2020 20:15

dvoytenko suggested changes Sep 3, 2020

View reviewed changes

samouri reviewed Sep 8, 2020

View reviewed changes

krdwan force-pushed the date-countdown-preact branch from 3f378e0 to d9eefff Compare September 8, 2020 19:56

krdwan requested review from dvoytenko and samouri September 8, 2020 20:51

samouri reviewed Sep 8, 2020

View reviewed changes

extensions/amp-date-countdown/1.0/date-countdown.js Show resolved Hide resolved

jridgewell reviewed Sep 8, 2020

View reviewed changes

dvoytenko approved these changes Sep 9, 2020

View reviewed changes

extensions/amp-date-countdown/1.0/date-countdown.js Outdated Show resolved Hide resolved

krdwan requested review from samouri and jridgewell September 9, 2020 15:57

krdwan mentioned this pull request Sep 10, 2020

✨ [bento][amp-date-countdown] Initial Preact component for amp-date-countdown #30038

Closed

samouri approved these changes Sep 10, 2020

View reviewed changes

jridgewell approved these changes Sep 11, 2020

View reviewed changes

krdwan force-pushed the date-countdown-preact branch from 7a1ae87 to b480a76 Compare September 11, 2020 13:27

krdwan added 12 commits September 14, 2020 09:20

First commit Preact component

d762f45

Add clear comments for helper functions

c95583d

Various review comment updates

56c4e9e

More review updates

6ea8b86

More review updates

93f9de2

Add useMemo

1b6b5bc

Updates for code review

fd64bc6

Review comments and rebase

a5161d0

Remove rebase conflict

f1cfff0

remove quotes on playable

2cfca11

Remove date format from epoch

991f2ec

add quotes for object keys that are output to template

955c8db

krdwan force-pushed the date-countdown-preact branch from b480a76 to 955c8db Compare September 14, 2020 13:21

krdwan merged commit ec0f14c into ampproject:master Sep 14, 2020

ampprojectbot added the PR use: In Beta / Experimental label Sep 23, 2020

✨ [bento][amp-date-countdown] Initial Preact component for amp-date-countdown #30092

✨ [bento][amp-date-countdown] Initial Preact component for amp-date-countdown #30092

Conversation

krdwan commented Sep 2, 2020 • 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

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

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

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

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

krdwan commented Sep 3, 2020 • 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

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

krdwan Sep 8, 2020 • 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

krdwan commented Sep 8, 2020

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

krdwan Sep 9, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvoytenko left a comment

Choose a reason for hiding this comment

krdwan commented Sep 9, 2020

dvoytenko commented Sep 10, 2020

krdwan commented Sep 2, 2020 •

edited

krdwan commented Sep 3, 2020 •

edited

krdwan Sep 8, 2020 •

edited

krdwan Sep 9, 2020 •

edited