JavaScript cleanup

Is there a way I can clean this up? I repeat myself quite a bit here.

const quotesList: HTMLElement | null =
    document.querySelector("section#quotes ul");

const quotes: NodeListOf<Element> | null = document.querySelectorAll(
  "section#quotes ul li"
);

if (quotesList) {
  quotesList.addEventListener("mouseover", (event) => {
    const element = event.target as HTMLElement;
    const card = element.closest("li") as HTMLElement;

    if (!(element.tagName === "A")) return;

    const plum: SVGElement | null = card.querySelector("svg");

    if (plum) plum.style.transform = "rotate(-45deg)";
  });

  quotesList.addEventListener("mouseout", (event) => {
    const element = event.target as HTMLElement;
    const card = element.closest("li") as HTMLElement;

    if (!(element.tagName === "A")) return;

    const plum: SVGElement | null = card.querySelector("svg");

    if (plum) plum.style.transform = "rotate(0deg)";
  });
}

const quotesList: HTMLElement | null =
    document.querySelector("section#quotes ul");

const quotes: NodeListOf<Element> | null = document.querySelectorAll(
  "section#quotes ul li"
);

if (quotesList) {
  quotesList.addEventListener("mouseover", (event) => {
    const element = event.target as HTMLElement;
    const card = element.closest("li") as HTMLElement;

    if (!(element.tagName === "A")) return;

    const plum: SVGElement | null = card.querySelector("svg");

    if (plum) plum.style.transform = "rotate(-45deg)";
  });

  quotesList.addEventListener("mouseout", (event) => {
    const element = event.target as HTMLElement;
    const card = element.closest("li") as HTMLElement;

    if (!(element.tagName === "A")) return;

    const plum: SVGElement | null = card.querySelector("svg");

    if (plum) plum.style.transform = "rotate(0deg)";
  });
}

4 Replies

vince•12mo ago

Cleaned it up to:

const quotesList: HTMLElement | null =
  document.querySelector("section#quotes ul");

const quotes: NodeListOf<Element> | null = document.querySelectorAll(
  "section#quotes ul li"
);

if (quotesList) {
  quotesList.addEventListener("mouseover", (event) => {
    const plum = linkMouseEventSetup(event);
    if (!plum) return;

    plum.style.transform = "rotate(-45deg)";
  });

  quotesList.addEventListener("mouseout", (event) => {
    const plum = linkMouseEventSetup(event);
    if (!plum) return;

    plum.style.transform = "rotate(0deg)";
  });
}

const linkMouseEventSetup = (event: MouseEvent) => {
  const element = event.target as HTMLElement;
  const card = element.closest("li") as HTMLElement;

  if (!(element.tagName === "A")) return;

  const plum: SVGElement | null = card.querySelector("svg");

  return plum;
};

const quotesList: HTMLElement | null =
  document.querySelector("section#quotes ul");

const quotes: NodeListOf<Element> | null = document.querySelectorAll(
  "section#quotes ul li"
);

if (quotesList) {
  quotesList.addEventListener("mouseover", (event) => {
    const plum = linkMouseEventSetup(event);
    if (!plum) return;

    plum.style.transform = "rotate(-45deg)";
  });

  quotesList.addEventListener("mouseout", (event) => {
    const plum = linkMouseEventSetup(event);
    if (!plum) return;

    plum.style.transform = "rotate(0deg)";
  });
}

const linkMouseEventSetup = (event: MouseEvent) => {
  const element = event.target as HTMLElement;
  const card = element.closest("li") as HTMLElement;

  if (!(element.tagName === "A")) return;

  const plum: SVGElement | null = card.querySelector("svg");

  return plum;
};

Rägnar O'ock•12mo ago

There's no need to cast quoteList to HTMLElement | null typescript can infer that itself Same goes for quotes Instead of doing plum.querySelector('svg') and casting the return, you can do plum.getElementByTagName('svg') for the same reason Don't cast element and card (still the same reason) Instead of if (!(element.tagName === "A")) return; you can put what follows inside the if (and remove the !(...) Instead of

document . querySelecto rAll(
"section#quotes ul li")

you can do quoteList?.getElementsByTagName('li') So basically: - remove the type casts (you shouldn't need to use them). - don't use negative guard closes if that is the only thing in the block - use the most appropriate DOM query method for what you need (that has the double advantage of making the code clearer AND it tends to be more efficient (even if the performance differential here is pretty much inexistent))

Joao•12mo ago

I would say that sometimes you do need to cast some elements, but stay consistent in how you do. Sometimes you use the as keyword while other times you declare a variable and the type all at once. Try to be as consistent as possible. You also don't need to query section#quotes since id's are, by definition, unique. So you can simply do document.querySelector("#quotes"). You can try to use the ? operator to skip one explicit check. And since you don't actually use the card except for further querying down the DOM you can use method chaining. Normally, I would prefer early return to avoid nesting but since there's only a single instruction it makes sense to put all the conditions under the same if statement.

quotesList.addEventListener("mouseover", (event) => {
    const element = event.target as HTMLElement;
    const plum = element.closest('li')?.querySelector("svg");

    if (element && plum && element.tagName === "A") {
        plum.style.transform = "rotate(-45deg)";
    }
});

quotesList.addEventListener("mouseover", (event) => {
    const element = event.target as HTMLElement;
    const plum = element.closest('li')?.querySelector("svg");

    if (element && plum && element.tagName === "A") {
        plum.style.transform = "rotate(-45deg)";
    }
});

vince•12mo ago

Thank you so much guys! This is exactly why I asked. I knew my JS and TS needed a lot of work haha. I'll try this stuff out when I get on my computer 🙂

Gaming

Programming

JavaScript cleanup