fix spacing bugs with item badges

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

@@ -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>
   );
 }
 

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>
   );
 }

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>
               }
             />

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>
   );
 }

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"
-      >
+    <Wrap spacing="2" opacity="0.7" {...props}>
       {children}
-      </Box>
-    </Box>
-  );
-}
-
-export function ItemBadge({ children, ...props }) {
-  return (
-    <Badge margin="1" {...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>
   );
 }