Refactor about popup code (#229)

* Renames variables/classes to be more clear * Uses `hidden` rather than `display: none`. Claude told me yesterday that `hidden` is better. The code is more clear also.
ParkingReformNetwork · Jul 10, 2024 · 43a9987 · 43a9987
1 parent 879b0c7
commit 43a9987
Show file tree

Hide file tree

Showing 5 changed files with 25 additions and 31 deletions.
diff --git a/index.html b/index.html
@@ -104,15 +104,12 @@
       </nav>
     </header>
 
-    <section class="about-text-popup" aria-label="about the map">
-      <div class="about-close">
-        <i
-          class="close-icon fa-regular fa-circle-xmark"
-          title="close about popup"
-        ></i>
+    <section class="about-popup" aria-label="about the map" hidden>
+      <div class="about-popup-close-icon-container">
+        <i class="fa-regular fa-circle-xmark" title="close the about popup"></i>
       </div>
       <h2>Parking Lot Coverage Map</h2>
-      <div id="lots-toggle" style="display: none">
+      <div id="lots-toggle" hidden>
         <h3 id="lots-toggle-on"><a>Click me turn on lot data.</a></h3>
         <h3 id="lots-toggle-off"><a>Click me turn off lot data.</a></h3>
       </div>

diff --git a/src/css/_about.scss b/src/css/_about.scss
@@ -4,13 +4,12 @@ dd {
   margin-bottom: 10px;
 }
 
-.about-text-popup {
+.about-popup {
   position: absolute;
   z-index: 2001;
   background-color: colors.$white;
   font-size: 18px;
   border: 1px solid colors.$gray-translucent;
-  display: none;
   left: 10%;
   height: 60%;
   width: 80%;
@@ -27,7 +26,7 @@ dd {
   color: colors.$gray;
 }
 
-.about-close {
+.about-popup-close-icon-container {
   float: right;
   cursor: pointer;
 }
diff --git a/src/js/about.ts b/src/js/about.ts
@@ -4,38 +4,36 @@
  * Set up event listeners to open and close the about popup.
  */
 const setUpAbout = () => {
-  const aboutElement = document.querySelector(".about-text-popup");
-  const infoButton = document.querySelector(".header-about-icon-container");
+  const aboutPopup = document.querySelector(".about-popup");
+  const aboutHeaderIcon = document.querySelector(
+    ".header-about-icon-container"
+  );
   if (
-    !(aboutElement instanceof HTMLElement) ||
-    !(infoButton instanceof HTMLElement)
+    !(aboutPopup instanceof HTMLElement) ||
+    !(aboutHeaderIcon instanceof HTMLElement)
   )
     return;
 
-  infoButton.addEventListener("click", () => {
-    aboutElement.style.display =
-      aboutElement.style.display !== "block" ? "block" : "none";
+  aboutHeaderIcon.addEventListener("click", () => {
+    aboutPopup.hidden = !aboutPopup.hidden;
   });
 
   // closes window on clicks outside the info popup
   window.addEventListener("click", (event) => {
     if (
+      !aboutPopup.hidden &&
       event.target instanceof Element &&
-      !infoButton.contains(event.target) &&
-      aboutElement.style.display === "block" &&
-      !aboutElement.contains(event.target)
+      !aboutHeaderIcon.contains(event.target) &&
+      !aboutPopup.contains(event.target)
     ) {
-      aboutElement.style.display = "none";
-      infoButton.classList.toggle("active");
+      aboutPopup.hidden = true;
     }
   });
 
-  const aboutClose = document.querySelector(".about-close");
-  if (!(aboutClose instanceof HTMLElement)) return;
-  aboutClose.addEventListener("click", () => {
-    // Note that the close element will only render when the about text popup is rendered.
-    // So, it only ever makes sense for a click to close.
-    aboutElement.style.display = "none";
+  const closeIcon = document.querySelector(".about-popup-close-icon-container");
+  if (!(closeIcon instanceof HTMLElement)) return;
+  closeIcon.addEventListener("click", () => {
+    aboutPopup.hidden = true;
   });
 };
 

diff --git a/src/js/setUpSite.ts b/src/js/setUpSite.ts
@@ -230,7 +230,7 @@ const setUpParkingLotsLayer = async (
   const toggle: HTMLAnchorElement | null =
     document.querySelector("#lots-toggle");
   if (toggle) {
-    toggle.style.display = "block";
+    toggle.hidden = false;
   }
   document.querySelector("#lots-toggle-off")?.addEventListener("click", () => {
     parkingLayer.removeFrom(map);

diff --git a/tests/app/about.test.ts b/tests/app/about.test.ts
@@ -6,7 +6,7 @@ test("about popup can be opened and closed", async ({ page }) => {
   const aboutIcon = ".header-about-icon-container";
 
   const aboutIsVisible = async () =>
-    page.$eval(".about-text-popup", (el) => el.style.display === "block");
+    page.$eval(".about-popup", (el) => el instanceof HTMLElement && !el.hidden);
 
   // before click
   expect(await aboutIsVisible()).toBe(false);
@@ -24,7 +24,7 @@ test("about popup can be opened and closed", async ({ page }) => {
   expect(await aboutIsVisible()).toBe(true);
 
   // click x icon in popup
-  await page.click(".about-close");
+  await page.click(".about-popup-close-icon-container");
   expect(await aboutIsVisible()).toBe(false);
 
   // click about icon (open popup)