[MWPW-153363] Countdown Timer - CDT #2803

rahulgupta999 · 2024-08-29T08:07:29Z

As presented in AST, the regions have been using a ticker/timer/countdown visual to boost click through rates and engagement as an enticement for certain promotions; e.g., Black Friday / Cyber Monday

Countdown Timer (CDT)
Properties:

CDT Layout: horizontal (default), vertical
CDT Color mode: light (default), dark
CDT End Date: input as YYYY-MM-DD, dd:hh:mm, time zone code
CDT Start Date: input as YYYY-MM-DD, dd:hh:mm, time zone code

Resolves: MWPW-153363

Test URLs:

aem-code-sync · 2024-08-29T08:07:55Z

	Page	Scores	Audits	Google
M	/drafts/rahulgup/marquee-parent?martech=off
D	/drafts/rahulgup/marquee-parent?martech=off

codecov · 2024-08-29T08:11:20Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.10%. Comparing base (510f7a6) to head (bf6a427).
Report is 47 commits behind head on stage.

Additional details and impacted files

@@            Coverage Diff             @@
##            stage    #2803      +/-   ##
==========================================
+ Coverage   95.87%   96.10%   +0.22%     
==========================================
  Files         173      216      +43     
  Lines       46333    54216    +7883     
==========================================
+ Hits        44421    52102    +7681     
- Misses       1912     2114     +202

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

libs/blocks/countdown-timer/countdown-timer.js

libs/features/mas/web-components/build.mjs

libs/features/mas/web-components/src/cdt/countdown-timer.css.js

libs/features/mas/web-components/src/cdt/countdown-timer.js

github-actions · 2024-08-30T01:23:55Z

This pull request is not passing all required checks. Please see this discussion for information on how to get all checks passing. Inconsistent checks can be manually retried. If a test absolutely can not pass for a good reason, please add a comment with an explanation to the PR.

libs/blocks/countdown-timer/countdown-timer.js

yesil

Looks good to me.

With https://github.com/orgs/adobecom/discussions/2819#discussioncomment-10544162, the LCP impact could be even further reduced.

cc: @narcis-radu @mokimo

no longer requesting changes

narcis-radu · 2024-09-09T07:53:13Z

libs/blocks/countdown-timer/countdown-timer.js

+        width: 100%;
+        font-size: 14px;
+        font-weight: regular;
+        text-align: left;


what about RTL locales?

start is likely a better option here, as @narcis-radu mentioned - https://developer.mozilla.org/en-US/docs/Web/CSS/text-align

narcis-radu · 2024-09-09T07:53:23Z

libs/blocks/countdown-timer/countdown-timer.js

+    }
+
+    .horizontal .timer-block {
+        margin-left: 10px;


this might create issues on RTL locales

narcis-radu · 2024-09-09T07:56:28Z

libs/blocks/media/media.js

@@ -33,6 +33,14 @@ function decorateQr(el) {
  qrImage.classList.add('qr-code-img');
 }

+export async function loadCountDownTimer(el) {


I see the same method in both marquee and media; what do you think about adding it to decorate.js and load it conditionally?

narcis-radu · 2024-09-09T07:58:02Z

libs/blocks/countdown-timer/countdown-timer.js

+    }
+
+    .timer-label {
+        font-size: 16px;


aren't the font sizes supposed to be consistent with existing variables?

narcis-radu · 2024-09-09T07:58:11Z

libs/blocks/countdown-timer/countdown-timer.js

+
+    .timer-label {
+        font-size: 16px;
+        font-weight: bold;


we should use numeric value

mokimo

I won't block the PR//request changes anymore - given the business priority, however I still don't agree with how a few things been done. Milo has a certain coding style and patterns, which this block does not follow.

This block is written in a very different way compared to other blocks.
- It attaches itself to the shadowRoot, which is a very novel pattern
- It inlines styles
- It uses private class variables, which I haven't ever seen before
It's a performance anti-pattern to add loading more things to the LCP block IMO
The CDT block could be loaded asynchronously, but you would need to work around the potential CLS. That way you wouldn't impact performance.
Nested blocks are bad for several reasons and discouraged by AEM https://www.aem.live/docs/davidsmodel#rule-2-no-nested-blocks-for-authors

beside creating a CDT-Marquee which does not look great (or does it? temp/bin components folder?)
Current PR looks like the only solution to me together probably with preload/prefetches

I personally feel like a "Temp" CDT-Marquee would be a cleaner solution, also given that the marquee is a widely used block and we now add features to the block that might only be used on very special occasions during the year.

mokimo · 2024-09-09T09:35:18Z

As discussed with @yesil and @vhargrave last week, another good idea would be making this an autoblock which would then be re-useable across blocks by default.
That way you also avoid the loadBlock portion within the marquee

Priyankavgit · 2024-09-09T12:10:43Z

The testing status has been updated in the linked JIRA ticket: [MWPW-153363]
Apart from the hero-marquee, the rest of the changes have been fixed and are working properly.

vhargrave

I personally think we need to pull a hard brake with this implementation.
There are a lot of things going on here that are not milo standard and that additionally are not good practice for LCP.

declaring CSS in the .js file
setting innerHTML with content from our authors (security issue)
loading in content over a fragment in the LCP block
loading a block within a block

This is also not a typical use case, so I think we need to come up with a good solution for it. My opinion currently is that the best solution to add this would be to add it as a feature. Add metadata which defines the start & end date of this timer, and either add information to the metadata, to which block this feature should be added, or have a little code in the blocks that add this timer to the end of the text in the block.
This is just my best idea currently though, and there could be other better ideas.
cc: @mokimo @narcis-radu @overmyheadandbody @robert-bogos

P.S. I also think an autoblock could work here and would also be a decent solution, but it seems little hard for our authors to author. That's the only reason I'm not all for it.

overmyheadandbody

A few minor things on top of the over-arching feedback provided so far. This is not consistent with any of the current Milo practices and will be an issue for maintainability. What is the major advantage of this approach to justify going against other patterns?

overmyheadandbody · 2024-09-09T13:14:58Z

libs/blocks/countdown-timer/countdown-timer.js

+    .horizontal {
+        display: flex;
+        flex-direction: row;
+        padding: 20px 0px;
+    }
+
+    .vertical {
+        display: flex;
+        flex-direction: column;
+        padding: 20px 0px;
+    }


Suggested change

.horizontal {

display: flex;

flex-direction: row;

padding: 20px 0px;

}

.vertical {

display: flex;

flex-direction: column;

padding: 20px 0px;

}

.horizontal, .vertical {

display: flex;

padding: 20px 0;

}

.vertical {

flex-direction: column;

}

flex-direction is row by default, some repetition can be avoided here

overmyheadandbody · 2024-09-09T13:15:26Z

libs/blocks/countdown-timer/countdown-timer.js

+    }
+
+    .light .timer-label {
+        color: #000000;


Suggested change

color: #000000;

color: #000;

overmyheadandbody · 2024-09-09T13:15:34Z

libs/blocks/countdown-timer/countdown-timer.js

+    }
+
+    .dark .timer-label {
+        color: #FFFFFF;


Suggested change

color: #FFFFFF;

color: #FFF;

overmyheadandbody · 2024-09-09T13:15:49Z

libs/blocks/countdown-timer/countdown-timer.js

+    }
+
+    .horizontal .timer-label {
+        margin: 0px 2px 45px 2px;


Suggested change

margin: 0px 2px 45px 2px;

margin: 0px 2px 45px;

Also, why do you need the 45px? They seem to be taking up additional space, expanding the host block more than would be needed.

overmyheadandbody · 2024-09-09T13:16:22Z

libs/blocks/countdown-timer/countdown-timer.js

+        background-color: #222222;
+        color: #FFFFFF;


Suggested change

background-color: #222222;

color: #FFFFFF;

background-color: #222;

color: #FFF;

overmyheadandbody · 2024-09-09T13:21:36Z

libs/blocks/countdown-timer/countdown-timer.js

+
+  #timeRangesEpoch = [];
+
+  static css = `


Is the intent to avoid loading the CSS file? It's a well established pattern in Milo, I don't see why you'd want to add things here, since a request is still made for the styles. This goes against current conventions, is a risk for maintainability and likely doesn't improve performance, if that was the goal.

overmyheadandbody · 2024-09-09T13:24:14Z

libs/blocks/countdown-timer/countdown-timer.js

+
+  countdownCompleted() {
+    this.isVisible = false;
+    clearInterval(this.countdownInterval);


Couldn't you call disconnectedCallback instead? Also, the method name uses an adjective to define an action, disconnectCallback might be a better fit

overmyheadandbody · 2024-09-09T13:25:32Z

libs/blocks/countdown-timer/countdown-timer.js

+      this.shadowRoot.innerHTML = '';
+      return;
+    }
+
+    this.shadowRoot.innerHTML = `


Setting innerHTML will likely trigger some Kodiak alerts

overmyheadandbody · 2024-09-09T13:35:07Z

libs/blocks/countdown-timer/countdown-timer.js

+    el.replaceWith(cdt);
+  } catch (error) {
+    el.replaceWith('');
+    window.lana?.log(`Failed to load countdown timer module: ${error}`);


You should likely add some tags to this, latest format can be seen in this PR.

overmyheadandbody · 2024-09-09T13:55:40Z

libs/blocks/media/media.js

@@ -105,4 +113,8 @@ export default function init(el) {
  const mediaRowReversed = el.querySelector(':scope > .foreground > .media-row > div').classList.contains('text');
  if (mediaRowReversed) el.classList.add('media-reverse-mobile');
  decorateTextOverrides(el);
+
+  if (el.classList.contains('countdown-timer')) {
+    loadCountDownTimer(el);


Shouldn't this be awaited here as well?

Axelcureno · 2024-09-13T20:34:48Z

libs/blocks/countdown-timer/countdown-timer.js

@@ -0,0 +1,242 @@
+import { createTag } from '../../utils/utils.js';
+
+export class CountdownTimer extends HTMLElement {


I thought Milo blocks were not supposed to use classes and web components. Normally classes are defined in web-components folder.
What is the reason for doing this way?

mokimo · 2024-09-17T17:20:24Z

@npeltier @yesil @rahulgupta999 to not block you on this, if you do create a new temp-marquee specifically for the CDT to hit the business goals, that would be the easiest and quickest way to merge this (IMO) ... a new block or "block-valid-till-X" is easier to create and hopefully also clean up again

narcis-radu · 2024-09-18T16:00:19Z

I disagree with @mokimo's suggestion, as I believe at a general level this could lead to dependencies once the code intended for removal reaches production. We may face difficulties deprecating it if it's still being used on certain pages, the team will not see a RTB task as a top priority. Nevertheless, this discussion is more suitable for https://github.com/orgs/adobecom/discussions if we need to follow-up.
My suggestion is to investigate the previously shared feedback and see whether an auto block makes more sense here; if not, we need to address things in a way that they don't impact performance. Regardless of the approach, the code footprint should be small enough and should follow the general guidelines in Milo.

yesil · 2024-09-19T13:33:48Z

libs/blocks/countdown-timer/countdown-timer.js

+    }
+
+    this.shadowRoot.innerHTML = `
+      <style>${CountdownTimer.css}</style>


@rahulgupta999 you can load the CSS as at https://github.com/adobecom/milo/blob/stage/libs/blocks/bulk-publish-v2/components/bulk-publisher.js#L58

it is more aligned with Milo.

rahulgupta999 · 2024-09-20T13:04:11Z

Closing this PR as this has been replaced with approach to use page metadata
#2928

rahulgupta999 added 7 commits August 19, 2024 07:41

add block placeholder

24b9d01

Add default func

934a565

Add CDT LIT component

2089792

Fix CDT bug

6960b74

Add some basic styling

31ad9e7

some styling

15dd05a

Styling fixes

cc47eb3

rahulgupta999 requested a review from a team as a code owner August 29, 2024 08:07

rahulgupta999 marked this pull request as draft August 29, 2024 08:07

rahulgupta999 added 2 commits August 29, 2024 15:16

Add cdt

424ed9d

Lint fixes

aded0d3

rahulgupta999 added run-nala Run Nala Test Automation against PR new-feature New block or other feature labels Aug 29, 2024

rahulgupta999 requested review from yesil, Axelcureno, 3ch023 and npeltier August 29, 2024 12:59

yesil reviewed Aug 29, 2024

View reviewed changes

rahulgupta999 added 4 commits August 29, 2024 19:06

cr comments

701db3e

review comment

3735250

RC

06f69df

RC

a747963

rahulgupta999 added 5 commits August 30, 2024 11:06

Link CDT to marquee

148e00b

added light/dark support

0c4fbfd

added div ids

ab3b7f3

fix unit test

3034ebc

fix

53c29c3

rahulgupta999 added 4 commits September 5, 2024 21:07

Add test for coverage

ed8f705

code coverage

559282a

Fix unit tests

683b210

code coverage complete

127dcc4

yesil reviewed Sep 6, 2024

View reviewed changes

libs/blocks/countdown-timer/countdown-timer.js Outdated Show resolved Hide resolved

libs/blocks/countdown-timer/countdown-timer.js Outdated Show resolved Hide resolved

libs/blocks/countdown-timer/countdown-timer.js Outdated Show resolved Hide resolved

move styles in the block,

264e7ef

yesil reviewed Sep 6, 2024

View reviewed changes

libs/blocks/countdown-timer/countdown-timer.js Outdated Show resolved Hide resolved

Add exception for date.parse

4a2ea64

yesil approved these changes Sep 6, 2024

View reviewed changes

rahulgupta999 added 2 commits September 6, 2024 20:18

Support of CDT for modal

9b7d305

add testcase for media

104abd8

mokimo self-requested a review September 6, 2024 14:55

rahulgupta999 added 4 commits September 6, 2024 20:28

unused code

4e1cd96

fix for vertical layout in marquee

bda37e8

Merge remote-tracking branch 'upstream/stage' into cdt

a768246

CSS fixes for marquee centre

bf6a427

narcis-radu reviewed Sep 9, 2024

View reviewed changes

mokimo reviewed Sep 9, 2024

View reviewed changes

Priyankavgit added the verified PR has been E2E tested by a reviewer label Sep 9, 2024

vhargrave reviewed Sep 9, 2024

View reviewed changes

overmyheadandbody reviewed Sep 9, 2024

View reviewed changes

Axelcureno reviewed Sep 13, 2024

View reviewed changes

yesil reviewed Sep 19, 2024

View reviewed changes

rahulgupta999 closed this Sep 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MWPW-153363] Countdown Timer - CDT #2803

[MWPW-153363] Countdown Timer - CDT #2803

rahulgupta999 commented Aug 29, 2024 •

edited

Loading

aem-code-sync bot commented Aug 29, 2024 •

edited

Loading

codecov bot commented Aug 29, 2024 •

edited

Loading

github-actions bot commented Aug 30, 2024

yesil left a comment

narcis-radu Sep 9, 2024

overmyheadandbody Sep 9, 2024

narcis-radu Sep 9, 2024

narcis-radu Sep 9, 2024

narcis-radu Sep 9, 2024

narcis-radu Sep 9, 2024

mokimo left a comment •

edited

Loading

mokimo commented Sep 9, 2024 •

edited

Loading

Priyankavgit commented Sep 9, 2024

vhargrave left a comment •

edited

Loading

overmyheadandbody left a comment

overmyheadandbody Sep 9, 2024

overmyheadandbody Sep 9, 2024

overmyheadandbody Sep 9, 2024

overmyheadandbody Sep 9, 2024

overmyheadandbody Sep 9, 2024

overmyheadandbody Sep 9, 2024

overmyheadandbody Sep 9, 2024

overmyheadandbody Sep 9, 2024

overmyheadandbody Sep 9, 2024

overmyheadandbody Sep 9, 2024

Axelcureno Sep 13, 2024

mokimo commented Sep 17, 2024

narcis-radu commented Sep 18, 2024 •

edited

Loading

yesil Sep 19, 2024

rahulgupta999 commented Sep 20, 2024

		@@ -0,0 +1,242 @@
		import { createTag } from '../../utils/utils.js';

		export class CountdownTimer extends HTMLElement {

[MWPW-153363] Countdown Timer - CDT #2803

[MWPW-153363] Countdown Timer - CDT #2803

Conversation

rahulgupta999 commented Aug 29, 2024 • edited Loading

aem-code-sync bot commented Aug 29, 2024 • edited Loading

codecov bot commented Aug 29, 2024 • edited Loading

Codecov Report

github-actions bot commented Aug 30, 2024

yesil left a 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

mokimo left a comment • edited Loading

Choose a reason for hiding this comment

mokimo commented Sep 9, 2024 • edited Loading

Priyankavgit commented Sep 9, 2024

vhargrave left a comment • edited Loading

Choose a reason for hiding this comment

overmyheadandbody left a 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

mokimo commented Sep 17, 2024

narcis-radu commented Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

rahulgupta999 commented Sep 20, 2024

rahulgupta999 commented Aug 29, 2024 •

edited

Loading

aem-code-sync bot commented Aug 29, 2024 •

edited

Loading

codecov bot commented Aug 29, 2024 •

edited

Loading

mokimo left a comment •

edited

Loading

mokimo commented Sep 9, 2024 •

edited

Loading

vhargrave left a comment •

edited

Loading

narcis-radu commented Sep 18, 2024 •

edited

Loading