Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implement common margin props #1792

Closed
Tracked by #1800
sungik-choi opened this issue Dec 14, 2023 · 0 comments · Fixed by #1785
Closed
Tracked by #1800

Implement common margin props #1792

sungik-choi opened this issue Dec 14, 2023 · 0 comments · Fixed by #1785
Assignees
@sungik-choi
Labels
feat Issue or PR related to a new feature

Comments

@sungik-choi
Copy link
Contributor

sungik-choi commented Dec 14, 2023 •
edited
Loading

Description

다양한 컴포넌트에서 사용할 수 있는 공용 margin prop을 구현합니다.

Reasons for suggestion

https://www.radix-ui.com/themes/docs/theme/layout#margin-props 에서 영감을 받았습니다.

margin 속성은 컴포넌트의 내부 스타일링(우리의 디자인 결정)에 영향을 미치는 속성이 아닙니다. 사용자 편의성을 위해서 일부 컴포넌트에서 margin을 쉽게 적용할 수 있는 속성을 지원하면 좋겠다고 판단했습니다. 이미 Text, Icon 컴포넌트에서 지원하고 있으며, 사용처에서 유용하게 사용되고 있습니다.

Proposed solution

margin prop을 지원하는 컴포넌트는 다음 인터페이스를 준수해야합니다.

interface MarginProps {
  m?: CSSProperties['margin']
  mx?: CSSProperties['margin']
  my?: CSSProperties['margin']
  mt?: CSSProperties['marginTop']
  mr?: CSSProperties['marginRight']
  mb?: CSSProperties['marginBottom']
  ml?: CSSProperties['marginLeft']
}
@layer components {
  .margin {
    --margin-all: var(--b-margin-all, 0);

    margin:
      var(--b-margin-top, var(--b-margin-y, var(--margin-all)))
      var(--b-margin-right, var(--b-margin-x, var(--margin-all)))
      var(--b-margin-bottom, var(--b-margin-y, var(--margin-all)))
      var(--b-margin-left, var(--b-margin-x, var(--margin-all)));
  }
}
  • 기존 Text, Icon 컴포넌트에도 margin prop이 자체 구현되어있습니다. 이를 마이그레이션합니다. 마이그레이션 코드 작성이 필요할 수 있습니다.
  • shorthand(m)만 지원할지, 전체 이름(margin)으로도 지원할지 고민입니다. 둘의 우선순위를 판단하기가 어려워, 우선은 shorthand만 지원하는 쪽으로 결정했습니다.

References

image
@sungik-choi sungik-choi added the status:need triage Issue or PR that need triage attention label Dec 14, 2023
@github-project-automation github-project-automation bot moved this to 📌 Backlog in Bezier React Dec 14, 2023
@sungik-choi sungik-choi added feat Issue or PR related to a new feature and removed status:need triage Issue or PR that need triage attention labels Dec 14, 2023
@sungik-choi sungik-choi self-assigned this Dec 14, 2023
@sungik-choi sungik-choi changed the title Implement common margin props [v2] Implement common margin props Dec 15, 2023
@sungik-choi sungik-choi changed the title [v2] Implement common margin props Implement common margin props Dec 15, 2023
This was referenced Dec 15, 2023
Closed
Closed
Merged
sungik-choi added a commit that referenced this issue Dec 18, 2023
@sungik-choi
Implement Box layout component (#1785)
7d98d23 
<!--
  How to write a good PR title:
- Follow [the Conventional Commits
specification](https://www.conventionalcommits.org/en/v1.0.0/).
  - Give as much context as necessary and as little as possible
  - Prefix it with [WIP] while it’s a work in progress
-->

## Self Checklist

- [x] I wrote a PR title in **English** and added an appropriate
**label** to the PR.
- [x] I wrote the commit message in **English** and to follow [**the
Conventional Commits
specification**](https://www.conventionalcommits.org/en/v1.0.0/).
- [x] I [added the
**changeset**](https://github.com/changesets/changesets/blob/main/docs/adding-a-changeset.md)
about the changes that needed to be released. (or didn't have to)
- [x] I wrote or updated **documentation** related to the changes. (or
didn't have to)
- [x] I wrote or updated **tests** related to the changes. (or didn't
have to)
- [x] I tested the changes in various browsers. (or didn't have to)
  - Windows: Chrome, Edge, (Optional) Firefox
  - macOS: Chrome, Edge, Safari, (Optional) Firefox

## Related Issue
<!-- Please link to issue if one exists -->

- Fixes #1792
- Fixes #1818

## Summary
<!-- Please brief explanation of the changes made -->

Implement Box layout component

## Details
<!-- Please elaborate description of the changes -->

자세한 내용은 이슈를 참고해주세요. 구현하며 아래와 같은 점들을 신경썼습니다.

- radix Theme를 참고하여 공통 margin props/style, 레이아웃 컴포넌트를 위한 layout
props/style 을 구현했습니다. 각 인터페이스를 지원하고자하는 컴포넌트에서는 인터페이스를 상속하고, 구현에
classname과 style을 추가하도록 했습니다.
- `Box` 컴포넌트를 상속받는 형태(e.g. Chakra UI)가 되지 않도록 신경썼습니다. 이렇게 될 경우 `Box`
컴포넌트가 말그대로 CSS superset의 형태가 되어버릴 수 있어 이런 케이스는 피하고자 했습니다(e.g. `Text` 의
내부 구현은 `Box` 이며, `Box` 는 `Text` 를 구현할 수 있도록 `typo` 속성을 가지고 있다). 가능한한
컴포넌트의 책임을 명확히 하고자 의도했습니다.
- 같은 맥락으로 `color` prop은 제외했습니다. 글자색을 변경하는 속성인데, 이 속성은 왠만하면 `Text` 를 통해서만
컨트롤하도록 강제하고 싶었습니다.
- color를 제외한 border, overflow, box-shadow 속성은 Box 컴포넌트가 가지고 있어도 레이아웃을
담당한다는 책임을 크게 흐트리진 않는 거 같다고 판단하여 사용성을 위해 추가했습니다.
- `as` 속성을 통해 변경할 수 있는 컴포넌트는 Shopify polaris의 케이스를 참고했습니다.
- 기존 컴포넌트들과 다르게 width, height 등 dimension property에 number를 넣으면 px를 붙여주는
로직(e.g. `ModalContent`)을 제외했습니다. primitive layout 컴포넌트라는 특성상, 애플리케이션에서
굉장히 많이 호출될 수 있습니다. 꼭 필요한 것이 아니라면 number 체크 로직을 제거하고자 했습니다.

### Breaking change? (Yes/No)
<!-- If Yes, please describe the impact and migration path for users -->

No

## References
<!-- Please list any other resources or points the reviewer should be
aware of -->

- #1792 
- #1818
@github-project-automation github-project-automation bot moved this from 📌 Backlog to ✅ Done in Bezier React Dec 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@sungik-choi sungik-choi

Labels
feat Issue or PR related to a new feature
Projects
No open projects
Bezier React
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Implement Box layout component sungik-choi/bezier-react
1 participant
@sungik-choi