From 70efd9970e6790d1dc5c4fafe7268a386249a69f Mon Sep 17 00:00:00 2001
From: "Yang Wooseong (Andrew)" <dntjddid1@gmail.com>
Date: Wed, 8 Feb 2023 21:51:16 +0900
Subject: [PATCH] Fix bug that first `StackItem` has `marginBefore` when
 preceding node is not JSX.Element (#1120)

* fix(stack): fix bug that first valid element is given marginBefore

* feat(stack): add test code when first child node of stack is not valid

* chore(changest): add changeset

* chore(stack): fix typo in test code
---
 .changeset/real-vans-tie.md                   |  5 +++
 .../src/components/Stack/Stack/Stack.test.tsx | 36 +++++++++++++++++++
 .../src/components/Stack/Stack/Stack.tsx      | 27 ++++++++------
 3 files changed, 57 insertions(+), 11 deletions(-)
 create mode 100644 .changeset/real-vans-tie.md

diff --git a/.changeset/real-vans-tie.md b/.changeset/real-vans-tie.md
new file mode 100644
index 0000000000..62055d0427
--- /dev/null
+++ b/.changeset/real-vans-tie.md
@@ -0,0 +1,5 @@
+---
+"@channel.io/bezier-react": patch
+---
+
+Fix bug that first StackItem has marginBefore when the preceding node is not valid element such as null or false
diff --git a/packages/bezier-react/src/components/Stack/Stack/Stack.test.tsx b/packages/bezier-react/src/components/Stack/Stack/Stack.test.tsx
index 4e321bb8ff..86c76969b5 100644
--- a/packages/bezier-react/src/components/Stack/Stack/Stack.test.tsx
+++ b/packages/bezier-react/src/components/Stack/Stack/Stack.test.tsx
@@ -4,6 +4,7 @@ import React from 'react'
 /* Internal dependencies */
 import { css } from 'Foundation'
 import { render } from 'Utils/testUtils'
+import { StackItem } from 'Components/Stack/StackItem'
 import { Stack } from './Stack'
 
 describe('Stack', () => {
@@ -52,4 +53,39 @@ describe('Stack', () => {
       expect(getByTestId('stack')).toHaveStyle({ 'background-color': 'red' })
     })
   })
+
+  it('gives marginBefore attribute to second stackItem if it is given some spacing', () => {
+    const spacing = 10
+
+    const { getByTestId } = render(
+      <Stack direction="horizontal" spacing={spacing}>
+        <StackItem testId="one" />
+        <StackItem testId="two" />
+        <StackItem testId="three" />
+      </Stack>,
+    )
+
+    expect(getByTestId('one')).not.toHaveStyle('--margin-before: 10px')
+    expect(getByTestId('two')).toHaveStyle('--margin-before: 10px')
+    expect(getByTestId('three')).toHaveStyle('--margin-before: 10px')
+  })
+
+  it('does not give marginBefore attribute to first stackItem if first node is not a valid ReactNode', () => {
+    const spacing = 10
+
+    const { getByTestId } = render(
+      <Stack direction="horizontal" spacing={spacing}>
+        { false }
+        { null }
+        abc
+        <StackItem testId="one" />
+        <StackItem testId="two" />
+        <StackItem testId="three" />
+      </Stack>,
+    )
+
+    expect(getByTestId('one')).not.toHaveStyle('--margin-before: 10px')
+    expect(getByTestId('two')).toHaveStyle('--margin-before: 10px')
+    expect(getByTestId('three')).toHaveStyle('--margin-before: 10px')
+  })
 })
diff --git a/packages/bezier-react/src/components/Stack/Stack/Stack.tsx b/packages/bezier-react/src/components/Stack/Stack/Stack.tsx
index ef8d7fb983..9e940d1ba1 100644
--- a/packages/bezier-react/src/components/Stack/Stack/Stack.tsx
+++ b/packages/bezier-react/src/components/Stack/Stack/Stack.tsx
@@ -4,6 +4,7 @@ import React, {
   cloneElement,
   forwardRef,
   isValidElement,
+  useRef,
 } from 'react'
 import type { Ref } from 'react'
 
@@ -45,6 +46,8 @@ export const Stack = forwardRef(function Stack(
   }: StackProps,
   forwardedRef: Ref<HTMLElement>,
 ) {
+  const firstValidElementIdx = useRef(-1)
+
   return (
     <Styled.Container
       ref={forwardedRef}
@@ -60,24 +63,26 @@ export const Stack = forwardRef(function Stack(
     >
       { Children.map(
         children,
-        (element, index) => (
-          isValidElement(element)
-            /**
+        (element, index) => {
+          if (!isValidElement(element)) { return element }
+
+          /**
              * NOTE: this assumes that this element is `StackItem`.
              *
              * Even if the child is not a `StackItem` component,
              * it could forward the prop to `StackItem` deeper in the tree,
              * or implement a custom behavior compatible with `StackItemProps`.
              */
-            ? cloneElement(element, {
-              ...element.props,
-              direction,
-              marginBefore:
+          if (firstValidElementIdx.current === -1) { firstValidElementIdx.current = index }
+          return cloneElement(element, {
+            ...element.props,
+            direction,
+            marginBefore:
                 element.props.marginBefore ??
-                (index > 0 ? spacing : 0),
-            })
-            : element
-        ),
+                (index > firstValidElementIdx.current ? spacing : 0),
+          })
+        },
+
       ) }
     </Styled.Container>
   )