2
\$\begingroup\$

Please consider evaluating the code below for best practices, efficiency, and mistakes. I got some of the code from W3 schools (I know, I know, but they've come a long way) and assimilated it to what I need. Particularly, the days = hours = minutes = seconds = 0; is likely going to cause people stress but after research and evaluation, I found that it works well in this case.

Please evaluate my JS code. Thank you. CODEPEN

IMPRORTANT: The code below checks for the existence of an ACF variable (exposed via the functions.php with a WordPress backend), upon which this entire code is based. I replaced the variable with an example string which would be returned by this field.

(function () {
    let acf_vars = {timer: "2021 1 21 13 08 00"}; // this code is a replacement for the ACF variable

    if (!acf_vars.timer) {
        return document
            .querySelectorAll('.mobile-ticker')
            .forEach((container) => (container.style.display = 'none'));
    }

    const backendTimer = acf_vars.timer.split(' ');
    const y = backendTimer[0];
    const M = +backendTimer[1] - 1; // monthIndex in Date Object begins with 0, so we subtract 1
    const d = backendTimer[2];
    const h = backendTimer[3];
    const m = backendTimer[4];
    const s = backendTimer[5];

    // new Date(year, monthIndex [, day [, hours [, minutes [, seconds [, milliseconds]]]]])
    const countDownDate = new Date(y, M, d, h, m, s, 0).getTime();

    const timerInterval = setInterval(() => {
        const now = new Date().getTime();
        const distance = countDownDate - now;
        const expiredTimer = distance <= 0;

        let days = Math.floor(distance / (1000 * 60 * 60 * 24));
        let hours = Math.floor((distance % (1000 * 60 * 60 * 24)) / (1000 * 60 * 60));
        let minutes = Math.floor((distance % (1000 * 60 * 60)) / (1000 * 60));
        let seconds = Math.floor((distance % (1000 * 60)) / 1000);

        if (expiredTimer) {
            days = hours = minutes = seconds = 0;
            clearInterval(timerInterval);
        }

        if (typeof window.innerWidth === 'number' && window.innerWidth < 768) {
            document.querySelector('.ticker-mobile #days').innerHTML = days;
            document.querySelector('.ticker-mobile #hours').innerHTML = hours;
            document.querySelector('.ticker-mobile #minutes').innerHTML = minutes;
            document.querySelector('.ticker-mobile #seconds').innerHTML = seconds;
        } else {
            document.querySelector('.ticker-desktop #days').innerHTML = days;
            document.querySelector('.ticker-desktop #hours').innerHTML = hours;
            document.querySelector('.ticker-desktop #minutes').innerHTML = minutes;
            document.querySelector('.ticker-desktop #seconds').innerHTML = seconds;
        }
    }, 1000);
})();
\$\endgroup\$
3
\$\begingroup\$

Can you send the timestamp instead? Manipulating dates in JS is a bit tedious without a library. This is probably pretty clear to you already from having to declare all the y M d h m s variables. PHP, on the other hand, has createFromFormat, which has a flexible and friendly API. For example, I believe you could pass a formatting string of 'Y m d H i s' (corresponding to the input of '2021 1 4 13 08 00') and get the timestamp out of it immediately.

Working with timestamps is easier than working with custom formatted strings anyway. I don't know the source of the original date string, but I'd turn it into a timestamp as soon as possible once your app receives it (and, for example, save the timestamp in the database instead of the string).

When you do have to pass around a string instead of a timestamp, use a standardized ISO 8601 string, which can be parsed automatically in most languages.

Ticker display Currently, the tickers are hidden via the JS if there's no timer. Consider if you could carry out this check (and this whole conditional code) in the PHP, rather than sending the HTML markup regardless and then possibly hide it if there's no timer. For example, you might change it so that the server only serves this .js if there's a timer to begin with, otherwise the <script> tag (and maybe the tickers) are omitted entirely by the backend.

Destructure If you do need to parse the date string on the frontend, you can make things more concise by destructuring instead of listing all the indicies of backendTimer. I'd also at least use month and minutes instead of M and m (you might also expand the other variable names for consistency):

const [y, month, d, h, minutes, s] = acf_vars.timer.split(' ');
// The month in the input string is 1-indexed
// The month the date constructor accepts is 0-indexed, so subtract 1:
const countDownDate = new Date(y, month - 1, d, h, minutes, s).getTime();

The number of milliseconds can be omitted entirely when passing to the Date constructor.

Show the timer immediately Right now, the tickers start getting populated 1000ms after this script runs. Until then, they'll be empty. Consider putting the interval callback into a named function first, and call that function immediately, so that the user doesn't have to wait an additional second. Eg

const updateCountdown = () => {
  const now = new Date().getTime();
  // ...
};
const timerInterval = setInterval(updateCountdown, 1000);
updateCountdown();

Duplicate IDs are invalid HTML There should only ever be at most one element with a given ID in a document. If a certain sort of element might be repeated, use a class instead of an ID. But, even better:

Can you combine the tickers? You have one ticker for mobile, and a separate ticker for desktop. This is odd and results in code repetition. Unless there's a good reason not to, figure out if there's any way they can be combined. (I don't see anything substantial differentiating the two tickers in the code given, so I'm not sure what the obstacle might be. If it's just for CSS, use media queries instead.)

Mobile ticker vs ticker mobile? The HTML is a bit confusing:

<section class="mobile-ticker">
  <div class="ticker ticker-desktop">
    ...
  </div>
    <div class="ticker ticker-mobile">

So there's .mobile-ticker, the container, and then there's a .ticker-desktop, a child, and then there's a .ticker-mobile, another child. Maybe call the container something like ticker-container instead?

Only use .innerHTML when deliberately working with HTML markup. If what you have or want is plain text, it's faster, safer, and more semantically appropriate to use .textContent instead of .innerHTML.

\$\endgroup\$
2
  • \$\begingroup\$ This is really good, thank you very much for the detailed response. I'm implementing some of these changes as we speak, but the element's innerHTML/textContent doesn't seem to be available when putting the interval callback in a named function first: Uncaught TypeError: Cannot set property 'innerHTML' of null. In order to fix this, one option I found was to replace the IIFE with jQuery(function() {}), but I don't fully understand why this would be an issue in the first place \$\endgroup\$ – Hewe Jan 5 at 21:15
  • \$\begingroup\$ Your script is running before the element exists. See here. My preferred solution is to give the <script> tag the defer attribute. \$\endgroup\$ – CertainPerformance Jan 5 at 21:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.