Skip to content

Conversation

@sungik-choi
Copy link
Contributor

@sungik-choi sungik-choi commented Jul 26, 2022

Self Checklist

  • I wrote a PR title in English.
  • I added an appropriate label to the PR.
  • I wrote a commit message in English.
  • I wrote a commit message according to the Conventional Commits specification.
  • I added the appropriate changeset for the changes.
  • I wrote a unit test about the implementation.
  • I wrote a storybook document about the implementation.
  • I tested the implementation in various browsers.
    • Windows: Chrome, Edge, (Optional) Firefox
    • macOS: Chrome, Edge, Safari, (Optional) Firefox

Related Issue

없음

Summary

FormGroup 컴포넌트의 동작을 #866 이전과 동일하게 맞추기 위해, CSS Flex 프로퍼티 기본 값과 동일하게 세팅합니다.

Details

What are the default values for justify-content & align content?

The first one related to flexbox (the one you have to follow) gives the initial value as flex-start for justify-content and stretch for align-items. This is the Flexbox Level 1 specification and it's widely supported.

The second one is related to future alignment techniques. This specification is still in Draft mode and will define new way to align items in different contexts (Flexbox/Grid/Block/multicol/.. containers). The default value is normal for both properties (justify-content and align-items)

If you continue the reading you can see that normal will fallback to stretch in the flexbox context and for justify-content stretch behave as flex-start

So in all the cases, it's safe to assume that the initial value is flex-start for justify-content since normal will fallback to it (same for align-items where you can consider stretch as default)

위를 참고하여, justify-content 의 경우 flex-start 를 기본값으로, align-items 의 경우 stretch 를 기본값으로 하도록 변경합니다.

  • Stack 의 align 속성의 기본값을 마찬가지로 flex-start 에서 justify-content 로 변경하는 걸 제안해봅니다. 사용하다보니, 특히 VStack 의 경우 justify="stretch" 속성을 주는 경우가 굉장히 빈번했었습니다. 다른 분들의 의견도 궁금합니다. (FYI @inhibitor1217)

Breaking change or not (Yes/No)

No

References

  • Details 참고

@sungik-choi sungik-choi self-assigned this Jul 26, 2022
@changeset-bot
Copy link

changeset-bot bot commented Jul 26, 2022

🦋 Changeset detected

Latest commit: e0aa54a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@channel.io/bezier-react Patch
bezier-figma-plugin Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions
Copy link
Contributor

Chromatic Report

🚀 Congratulations! Your build was successful!

@codecov
Copy link

codecov bot commented Jul 26, 2022

Codecov Report

Merging #901 (e0aa54a) into next-v1 (8ee1798) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           next-v1     #901   +/-   ##
========================================
  Coverage    69.01%   69.01%           
========================================
  Files          205      205           
  Lines         2872     2872           
  Branches       791      791           
========================================
  Hits          1982     1982           
  Misses         775      775           
  Partials       115      115           
Impacted Files Coverage Δ
...react/src/components/Forms/FormGroup/FormGroup.tsx 100.00% <ø> (ø)

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 8ee1798...e0aa54a. Read the comment docs.

@sungik-choi sungik-choi added the fix PR related to bug fix label Jul 26, 2022
Copy link
Contributor

@Seolhun Seolhun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changeset 커밋이 마지막에 가야하지 않을까 싶네요. squash라 상관없겠지만요.

Copy link
Contributor

@inhibitor1217 inhibitor1217 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stack 인터페이스를 변경할 수도 있지만 breaking change 마이그레이션 비용도 고려가 필요합니다.
Discussion에서 얘기를 해 볼 수 있다고 생각합니다.

이 PR의 diff는 기존의 behavior를 복원하는 fix이기 때문에 괜찮습니다

Copy link
Contributor

@quino0627 quino0627 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR 머지되고 나면 데스크 대응 필요한 부분 수정하게 핑 주세요~ @sungik-choi

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix PR related to bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants