From 045fb95d69390ffea2ca640befa040e9b62a64b0 Mon Sep 17 00:00:00 2001
From: Matchu <matchu1993@gmail.com>
Date: Tue, 3 Nov 2020 20:11:37 -0800
Subject: [PATCH] fix spacing bugs with item badges
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

In a previous change, I moved the margin for item badges onto an ItemBadge element… but I didn't think through how that would break the spacing for the loading state of ItemPage. Now, the loading skeleton items _contained_ the badge margin, and so the spacing between badges was shiny skeleton-y.

Here, I replace ZoneBadgesList with a function that just returns the elements, and go back to using Chakra's Wrap component. That will apply the margin to direct children, and the zone badges are direct children now.

One option I'm thinking of in hindsight is an idea I had earlier: Chakra hacks the margin onto _React_ children, but could we use CSS direct child selector instead? A bit trickier to resolve the margin size to the theme's value, but plenty doable… something to consider!
---
 src/app/ItemPage.js            | 12 +++----
 src/app/ModelingPage.js        |  7 ++--
 src/app/UserItemsPage.js       |  9 +++---
 src/app/WardrobePage/Item.js   |  6 ++--
 src/app/components/ItemCard.js | 59 +++++++++++-----------------------
 5 files changed, 34 insertions(+), 59 deletions(-)

diff --git a/src/app/ItemPage.js b/src/app/ItemPage.js
index 8822dcb..a67fe26 100644
--- a/src/app/ItemPage.js
+++ b/src/app/ItemPage.js
@@ -2,6 +2,7 @@ import React from "react";
 import { css } from "emotion";
 import {
   AspectRatio,
+  Badge,
   Button,
   Box,
   IconButton,
@@ -28,7 +29,6 @@ import { useQuery, useMutation } from "@apollo/client";
 import { useParams } from "react-router-dom";
 
 import {
-  ItemBadge,
   ItemBadgeList,
   ItemThumbnail,
   NcBadge,
@@ -148,7 +148,7 @@ function ItemPageBadges({ item, isEmbedded }) {
   const searchBadgesAreLoaded = item?.name != null && item?.isNc != null;
 
   return (
-    <ItemBadgeList>
+    <ItemBadgeList marginTop="1">
       <SubtleSkeleton isLoaded={item?.isNc != null}>
         {item?.isNc ? <NcBadge /> : <NpBadge />}
       </SubtleSkeleton>
@@ -160,14 +160,14 @@ function ItemPageBadges({ item, isEmbedded }) {
             // empty).
             isLoaded={item.createdAt !== undefined}
           >
-            <ItemBadge
+            <Badge
               display="block"
               minWidth="5.25em"
               boxSizing="content-box"
               textAlign="center"
             >
               {item.createdAt && <ShortTimestamp when={item.createdAt} />}
-            </ItemBadge>
+            </Badge>
           </SubtleSkeleton>
         )
       }
@@ -249,7 +249,7 @@ function ItemPageBadges({ item, isEmbedded }) {
 
 function LinkBadge({ children, href, isEmbedded }) {
   return (
-    <ItemBadge
+    <Badge
       as="a"
       href={href}
       display="flex"
@@ -265,7 +265,7 @@ function LinkBadge({ children, href, isEmbedded }) {
         // window or not!
         isEmbedded ? <ExternalLinkIcon marginLeft="1" /> : <ChevronRightIcon />
       }
-    </ItemBadge>
+    </Badge>
   );
 }
 
diff --git a/src/app/ModelingPage.js b/src/app/ModelingPage.js
index 1208a69..7145581 100644
--- a/src/app/ModelingPage.js
+++ b/src/app/ModelingPage.js
@@ -1,5 +1,5 @@
 import React from "react";
-import { Box, Tooltip } from "@chakra-ui/core";
+import { Badge, Box, Tooltip } from "@chakra-ui/core";
 import gql from "graphql-tag";
 import { useQuery } from "@apollo/client";
 
@@ -7,7 +7,6 @@ import { Delay } from "./util";
 import HangerSpinner from "./components/HangerSpinner";
 import { Heading1, Heading2, usePageTitle } from "./util";
 import ItemCard, {
-  ItemBadge,
   ItemBadgeList,
   ItemCardList,
   NcBadge,
@@ -174,7 +173,7 @@ function ItemModelBadges({ item, currentUserOwnsItem }) {
       {item.isNc && <NcBadge />}
       {currentUserOwnsItem && <YouOwnThisBadge />}
       {item.speciesThatNeedModels.map((species) => (
-        <ItemBadge>{species.name}</ItemBadge>
+        <Badge>{species.name}</Badge>
       ))}
     </ItemBadgeList>
   );
@@ -192,7 +191,7 @@ function NewItemBadge({ createdAt }) {
       placement="top"
       openDelay={400}
     >
-      <ItemBadge colorScheme="yellow">New!</ItemBadge>
+      <Badge colorScheme="yellow">New!</Badge>
     </Tooltip>
   );
 }
diff --git a/src/app/UserItemsPage.js b/src/app/UserItemsPage.js
index 6b65303..8de90f7 100644
--- a/src/app/UserItemsPage.js
+++ b/src/app/UserItemsPage.js
@@ -17,7 +17,7 @@ import ItemCard, {
   NpBadge,
   YouOwnThisBadge,
   YouWantThisBadge,
-  ZoneBadgeList,
+  getZoneBadges,
 } from "./components/ItemCard";
 import useCurrentUser from "./components/useCurrentUser";
 import WIPCallout from "./components/WIPCallout";
@@ -259,10 +259,9 @@ function ClosetList({ closetList, isCurrentUser, showHeading }) {
                   {item.isNc ? <NcBadge /> : <NpBadge />}
                   {hasYouOwnThisBadge(item) && <YouOwnThisBadge />}
                   {hasYouWantThisBadge(item) && <YouWantThisBadge />}
-                  <ZoneBadgeList
-                    zones={item.allOccupiedZones}
-                    variant="occupies"
-                  />
+                  {getZoneBadges(item.allOccupiedZones, {
+                    variant: "occupies",
+                  })}
                 </ItemBadgeList>
               }
             />
diff --git a/src/app/WardrobePage/Item.js b/src/app/WardrobePage/Item.js
index cea5a02..ab262f5 100644
--- a/src/app/WardrobePage/Item.js
+++ b/src/app/WardrobePage/Item.js
@@ -21,7 +21,7 @@ import {
   NpBadge,
   YouOwnThisBadge,
   YouWantThisBadge,
-  ZoneBadgeList,
+  getZoneBadges,
 } from "../components/ItemCard";
 import SupportOnly from "./support/SupportOnly";
 import useSupport from "./support/useSupport";
@@ -231,8 +231,8 @@ function ItemBadges({ item }) {
       }
       {item.currentUserOwnsThis && <YouOwnThisBadge variant="short" />}
       {item.currentUserWantsThis && <YouWantThisBadge variant="short" />}
-      <ZoneBadgeList zones={occupiedZones} variant="occupied" />
-      <ZoneBadgeList zones={restrictedZones} variant="restricts" />
+      {getZoneBadges(occupiedZones, { variant: "occupies" })}
+      {getZoneBadges(restrictedZones, { variant: "restricts" })}
     </ItemBadgeList>
   );
 }
diff --git a/src/app/components/ItemCard.js b/src/app/components/ItemCard.js
index 7d1113e..a251345 100644
--- a/src/app/components/ItemCard.js
+++ b/src/app/components/ItemCard.js
@@ -199,34 +199,11 @@ export function ItemCardList({ children }) {
   );
 }
 
-export function ItemBadgeList({ children }) {
+export function ItemBadgeList({ children, ...props }) {
   return (
-    <Box marginTop="1" opacity="0.7">
-      <Box
-        // This is copying the styles of <Wrap spacing="2" />! The individual
-        // ItemBadges have their own `margin="1"`, which we counteract here for
-        // consistency.
-        //
-        // The difference between this and `Wrap` is that `Wrap` uses
-        // React.Children to wrap its children in containers with `margin="1"`,
-        // but that doesn't work when the badges aren't _direct_ children, like
-        // in `ZoneBadgeList`. The badges are what we want to have wrap, not
-        // whatever component happens to be the direct child!
-        margin="-1"
-        display="flex"
-        flexWrap="wrap"
-      >
-        {children}
-      </Box>
-    </Box>
-  );
-}
-
-export function ItemBadge({ children, ...props }) {
-  return (
-    <Badge margin="1" {...props}>
+    <Wrap spacing="2" opacity="0.7" {...props}>
       {children}
-    </Badge>
+    </Wrap>
   );
 }
 
@@ -245,9 +222,9 @@ export function ItemBadgeTooltip({ label, children }) {
 export function NcBadge() {
   return (
     <ItemBadgeTooltip label="Neocash">
-      <ItemBadge colorScheme="purple" display="block">
+      <Badge colorScheme="purple" display="block">
         NC
-      </ItemBadge>
+      </Badge>
     </ItemBadgeTooltip>
   );
 }
@@ -257,14 +234,14 @@ export function NpBadge() {
   //       default of inline-block.
   return (
     <ItemBadgeTooltip label="Neopoints">
-      <ItemBadge display="block">NP</ItemBadge>
+      <Badge display="block">NP</Badge>
     </ItemBadgeTooltip>
   );
 }
 
 export function YouOwnThisBadge({ variant = "long" }) {
   let badge = (
-    <ItemBadge
+    <Badge
       colorScheme="green"
       display="flex"
       alignItems="center"
@@ -272,7 +249,7 @@ export function YouOwnThisBadge({ variant = "long" }) {
     >
       <CheckIcon aria-label="Check" />
       {variant === "long" && <Box marginLeft="1">You own this!</Box>}
-    </ItemBadge>
+    </Badge>
   );
 
   if (variant === "short") {
@@ -284,7 +261,7 @@ export function YouOwnThisBadge({ variant = "long" }) {
 
 export function YouWantThisBadge({ variant = "long" }) {
   let badge = (
-    <ItemBadge
+    <Badge
       colorScheme="blue"
       display="flex"
       alignItems="center"
@@ -292,7 +269,7 @@ export function YouWantThisBadge({ variant = "long" }) {
     >
       <StarIcon aria-label="Star" />
       {variant === "long" && <Box marginLeft="1">You want this!</Box>}
-    </ItemBadge>
+    </Badge>
   );
 
   if (variant === "short") {
@@ -317,11 +294,11 @@ function ZoneBadge({ variant, zoneLabel }) {
       <ItemBadgeTooltip
         label={`Restricted: This item can't be worn with ${zoneLabel} items`}
       >
-        <ItemBadge>
+        <Badge>
           <Box display="flex" alignItems="center">
             {shorthand} <NotAllowedIcon marginLeft="1" />
           </Box>
-        </ItemBadge>
+        </Badge>
       </ItemBadgeTooltip>
     );
   }
@@ -329,15 +306,15 @@ function ZoneBadge({ variant, zoneLabel }) {
   if (shorthand !== zoneLabel) {
     return (
       <ItemBadgeTooltip label={zoneLabel}>
-        <ItemBadge>{shorthand}</ItemBadge>
+        <Badge>{shorthand}</Badge>
       </ItemBadgeTooltip>
     );
   }
 
-  return <ItemBadge>{shorthand}</ItemBadge>;
+  return <Badge>{shorthand}</Badge>;
 }
 
-export function ZoneBadgeList({ zones, variant }) {
+export function getZoneBadges(zones, propsForAllBadges) {
   // Get the sorted zone labels. Sometimes an item occupies multiple zones of
   // the same name, so it's important to de-duplicate them!
   let labels = zones.map((z) => z.label);
@@ -345,21 +322,21 @@ export function ZoneBadgeList({ zones, variant }) {
   labels = [...labels].sort();
 
   return labels.map((label) => (
-    <ZoneBadge key={label} zoneLabel={label} variant={variant} />
+    <ZoneBadge key={label} zoneLabel={label} {...propsForAllBadges} />
   ));
 }
 
 export function MaybeAnimatedBadge() {
   return (
     <ItemBadgeTooltip label="Maybe animated? (Support only)">
-      <ItemBadge
+      <Badge
         colorScheme="orange"
         display="flex"
         alignItems="center"
         minHeight="1.5em"
       >
         <Box as={HiSparkles} aria-label="Sparkles" />
-      </ItemBadge>
+      </Badge>
     </ItemBadgeTooltip>
   );
 }