sungik-choi commented 2 years ago

Summary

모든 컴포넌트의 토대가 되는 Box 컴포넌트를 구현합니다.

Details

Box 컴포넌트는 CSSProperty와 Foundation의 속성들을 prop으로 받는 만능 컴포넌트입니다. 모든 컴포넌트들은 Box를 extend하는 방식으로 구현될 예정입니다. 예를 들자면 아래와 같습니다.

/* 현재 bezier-react에 있는 컴포넌트 아님. 예시입니다 */
function Flex({ ... }) {
  return (
    <Box
      display="flex"
      { ... }
    >
    { ... }
  )
} 

function Center({ ... }) {
  return (
    <Flex
      alignItems="center"
      justifyContent="center"
      { ... }
    >
    { ... }
  )
}

bezier-react를 사용하는 프로젝트의 스타일링 방식을 바꿀 수 있는 변화라, 조금 조심스럽습니다. 하지만 이런 방식으로 스타일링하고자 하는 니즈는 이전부터 존재해왔다고 생각합니다. 현재 Text, Icon 컴포넌트에 있는 margin 관련 prop들이 이에 해당합니다. 이 prop들을 보면서 'margin 은 있는데 padding 은 왜 없지? prop으로 주입할 수 있는 CSS 속성과 아닌 CSS 속성의 차이점이 뭐지?' 라는 의문점을 가지게 되었습니다. 나아가선 '다른 CSS 속성 모두 prop으로 주면 안되나?' 라고 생각하게 되었습니다.

이번에 Foundation Spacing #677 을 추가하면서 prop으로 사용할 경우 장점이 더욱 명확해졌습니다. margin, padding을 preset으로 사용하게 될 경우 지금과 같은 사용 방식으론 아래와 같은 보일러 플레이트 형식의 Foundation 접근 코드가 스타일링하는 곳마다 작성되어야 합니다.

margin: ${({ foundation }) => foundation?.space?.['s4']};
padding: ${({ foundation }) => foundation?.space?.['s6']};

bezier-react의 존재 의의가 단순히 '베지어 디자인 시스템을 충실히 구현한다' 였기에 기존에 많은 UI 라이브러리에서 사용하고 있는 Box 와 같은 컴포넌트를 구현하지 않았던 거라 생각합니다. bezier-react(정확히는 src/components)에는 베지어 디자인 시스템에 존재하지 않는 컴포넌트가 있어선 안되기 때문입니다. 최근 논의를 거쳐 '베지어 디자인 시스템 ⊂ bezier-react' 라고 정의를 내렸습니다. 이제는 디자인 시스템의 스펙에 국한될 필요 없이 개발 생산성을 높이기 위한 컴포넌트들을 만들 수 있게 되었습니다. Box 컴포넌트를 만드는 것부터 시작해보면 어떨까 합니다.

이 PR이 마무리되면 Bezier의 모든 컴포넌트가 Box를 extend 하는 방식으로 변경할 예정입니다. 이후 Discussion에서 제안했던 Flex, Center 같은 Layout 컴포넌트들을 만들 예정입니다.

Pros & Cons

장점

사용처에서 Foundation에 직접 접근하여 스타일링하는 부분을(예: foundation?.theme?.['bg-black-light']) 제거할 수 있습니다. foundation?.*** 형태의 많은 불필요한 중복 코드가 제거됩니다.
모든 스타일링은 Box 컴포넌트를 통하므로, 이상적으로는 styled-component의 styled, css 또한 bezier-react에서 export할 필요가 없습니다(다른 UI 라이브러리들이 그러하듯이). 즉, 최종 결과물만 동일하다면 bezier-react 내부적으로 사용하는 스타일 라이브러리를 styled-component 에서 다른 라이브러리로 변경하더라도 사용처에서는 신경쓸 필요가 없습니다. 내부적으로 Box 컴포넌트를 어떻게 스타일링하는지만 신경쓰면 됩니다.
m, p, bg 같은 축약형 prop을 사용할 수 있습니다.

단점

컴포넌트의 prop에 스타일 속성이 섞여들어가므로, 컴포넌트의 return문이 길어져 가독성이 떨어질 수 있습니다.
- styled.attrs 메서드로 스타일 관련 속성을 분리해서 어느정도 해소할 수는 있겠으나, 그런 방식보단 사용처에서 모듈을 잘 나누는 방식으로 해결하는 방식을 지향해야한다고 생각합니다.
(성능 테스트 필요)

As-is & To-be

As-is

const Box = styled.section`
  display: flex;
  align-items: center;
  justify-content: center;
  padding: ${({ foundation }) => foundation?.spacing?.['s6']};
  margin: ${({ foundation }) => foundation?.spacing?.['s2']};
  background-color: ${({ foundation }) => foundation?.theme?.['bg-black-light']};
  ${Typography.Size15}
`

return (
  <Box>
    This is Box
  </Box>
)

To-be

return (
  <Box
    as="section"
    display="flex"
    alignItems="center"
    justifyContent="center"
    bg="bg-black-light"
    p="s6"
    m="s2"
    typo="t15" /* 이름은 논의 필요 */
  >
    This is Box
  </Box>
)

TODO

아래 Foundation의 요소를 Box 컴포넌트 내부에 녹여내야 합니다.

[ ] Border
[ ] Elevation
[ ] Roundings
[ ] Transition
[ ] Mixins
[ ] Typography를 key로 지정할 수 있도록
[ ] Psuedo Selector
[ ] Referring to other components
[ ] mx, my, px, py shorthand (mv, mh 등을 변경)

Browser Compatibility

OS / Engine 호환성을 반드시 확인해주세요.

Windows

[ ] Chrome - Blink
[ ] Edge - Blink
[ ] Firefox - Gecko (Option)
macOS
[ ] Chrome - Blink
[ ] Edge - Blink
[ ] Safari - WebKit
[ ] Firefox - Gecko (Option)

References

codecov[bot] commented 2 years ago

Codecov Report

Merging #681 (0a67d50) into next-v1 (dbbc57e) will decrease coverage by 1.00%. The diff coverage is 0.00%.

@@             Coverage Diff             @@
##           next-v1     #681      +/-   ##
===========================================
- Coverage    57.94%   56.94%   -1.01%     
===========================================
  Files          160      164       +4     
  Lines         2499     2543      +44     
  Branches       692      709      +17     
===========================================
  Hits          1448     1448              
- Misses         948      992      +44     
  Partials       103      103

Impacted Files	Coverage Δ
src/components/Layouts/Box/Box.styled.ts	`0.00% <0.00%> (ø)`
src/components/Layouts/Box/Box.tsx	`0.00% <0.00%> (ø)`
src/components/Layouts/Box/const.ts	`0.00% <0.00%> (ø)`
src/components/Layouts/Box/utils.ts	`0.00% <0.00%> (ø)`
src/utils/styleUtils.ts	`88.46% <0.00%> (-3.54%)`	:arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update dbbc57e...0a67d50. Read the comment docs.

Seolhun commented 2 years ago

간단한 의견 공유드립니다. 베지어에 반응형을 고려하면 위와 같은 Props는 확장성에 어려움을 겪을 수 있습니다. 간단한 예로, Mediaquery에서 flex가 row에서 column으로 바뀌어야 한다면 결국 css를 확장해야되는 수요가 생깁니다.

가장 걱정되는 concern은 만능형이라 어디서든 어떻게 쓰일 수 있어서 어떠한 형태까지 나오게될지 약간 두려운 부분이 있습니다.

sungik-choi commented 2 years ago

간단한 의견 공유드립니다. 베지어에 반응형을 고려하면 위와 같은 Props는 확장성에 어려움을 겪을 수 있습니다. 간단한 예로, Mediaquery에서 flex가 row에서 column으로 바뀌어야 한다면 결국 css를 확장해야되는 수요가 생깁니다.

가장 걱정되는 concern은 만능형이라 어디서든 어떻게 쓰일 수 있어서 어떠한 형태까지 나오게될지 약간 두려운 부분이 있습니다.

@Seolhun

현재 Draft 에서 반응형 디자인은 고려하지 않았어요. 추후 리서치해보며, 아래와 같은 방식으로 개선해볼 수 있을 거 같아요.

/* Chakra UI */
<>
  <Box
    height={{
      base: '100%', // 0-48em
      md: '50%', // 48em-80em,
      xl: '25%', // 80em+
    }}
    bg='teal.400'
    width={[
      '100%', // 0-30em
      '50%', // 30em-48em
      '25%', // 48em-62em
      '15%', // 62em+
    ]}
  />
  {/* responsive font size */}
  <Box fontSize={['sm', 'md', 'lg', 'xl']}>Font Size</Box>
  {/* responsive margin */}
  <Box mt={[2, 4, 6, 8]} width='full' height='24px' bg='tomato' />
  {/* responsive padding */}
  <Box bg='papayawhip' p={[2, 4, 6, 8]}>
    Padding
  </Box>
</>

/* MUI */
<Box
  component="img"
  sx={{
    height: 233,
    width: 350,
    maxHeight: { xs: 233, md: 167 },
    maxWidth: { xs: 350, md: 250 },
  }}
  alt="The house from the offer."
  src="https://images.unsplash.com/photo-1512917774080-9991f1c4c750?auto=format&w=350&dpr=2"
/>

/* styled-system */
<Box
  width={[
    1,    // 100% below the smallest breakpoint
    1/2,  // 50% from the next breakpoint and up
    1/4   // 25% from the next breakpoint and up
  ]}
/>

// responsive font size
<Box fontSize={[ 1, 2, 3, 4 ]} />

// responsive margin
<Box m={[ 1, 2, 3, 4 ]} />

// responsive padding
<Box p={[ 1, 2, 3, 4 ]} />

저는 단순히 styled 메서드 + CSS 문법을 통해 스타일링하던 방식을 그대로 컴포넌트의 prop으로 가져온 것뿐이라, 두려움을 가질 필요는 없다고 생각합니다. 이미 많은 UI 라이브러리들이 채택한 방식이기도 하구요. 방식이 조금 다를뿐 지금도 이미 styled HOC와 CSS Interpolation을 통해 그렇게 사용하고 있습니다. 단지 모듈이 ***.tsx 모듈과, ***.styled.ts 모듈로 분리되었을 뿐입니다. 저는 만능형이기때문에 경계해야한다기보다는, 재사용 가능한 컴포넌트를 제대로 분리하지 않고 사용하는 방식을 경계해야한다고 생각해요.

sungik-choi commented 2 years ago

Prop을 통해 스타일링 하는 방식을 사용한다면, 스타일 라이브러리를 styled-components를 emotion으로 변경하는 것도 충분히 고려할만한 거 같습니다.

Seolhun commented 2 years ago

@sungik-choi 개별 Component 내부에서 반응형을 고려할 수 있는 것은 좋은 방법이 아니라고 생각합니다.

첫 번째, tag와 display, box-sizing에 따라 다를 것입니다 특히, 저번에 공유드렸던 tag agnostic과 같은 시스템에서는 더 그렇게 될 것입니다. 이것이 2번과 같은 맥락을 합니다.

두 번째, Box의 BreakChange 혹은 변동사항이 현재 사용하고 있는 페이지에 어디까지 영향을 미칠지 모릅니다. 이것이 제가 말하는 가장큰 요인입니다. 각 컴포넌트들은 각각의 책임을 명확히해야 유지보수하기 편합니다. 위에 예제주신것을 기반으로 간단한 예제를 작성해보겠습니다.

const BoxText = styled(Box).attrs({
  forwardedAs: 'span',
})``

<BoxText
  bg='teal.400'
  width={[
    '100%', // ??
    '50%', // ??
    '25%', // ??
    '15%', // ??
  ]}
/>

위 코드는 거의 암호에 가깝습니다.

Extension을 사용하는 개발자마다 값이 다를 것입니다.
어디 부모 Element에 mount되는지에 따라 값이 다를 것입니다.
현재 Box코드에서 위 코드는 잘못된 코드가 아닙니다. 그렇다면, Box에 따른 가이드를 줘야한다는 것이 좋은 방법인지는 모르겠습니다.

sungik-choi commented 2 years ago

이건 #764 와 함께 진행해보겠습니다. 우선 Close 할게요.

sungik-choi commented 1 year ago

@Dogdriip

channel-io / bezier-react

PoC: Box Component #681

Summary

Details

Pros & Cons

장점

단점

As-is & To-be

As-is

To-be

TODO

[ ] Mixins

Browser Compatibility

Windows

macOS

References

Codecov Report