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.

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

Add focused style to Button component #1139

Merged

Conversation

sungik-choi
Copy link
Contributor

@sungik-choi sungik-choi commented Feb 21, 2023

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.
  • [Component] I wrote a unit test about the implementation.
  • [Component] I wrote a storybook document about the implementation.
  • [Component] I tested the implementation in various browsers.
    • Windows: Chrome, Edge, (Optional) Firefox
    • macOS: Chrome, Edge, Safari, (Optional) Firefox
  • [New Component] I added my username to the correct directory in the CODEOWNERS file.

Related Issue

없음

Summary

버튼에 포커스 스타일을 추가합니다.

Details

  • 기본적으로 버튼은 focus-visible 한 상태에서만 포커스 스타일을 가지도록 구현했습니다. 마우스 포인터로 포커스할 땐 포커스 스타일이 나타나지 않도록 하기 위해서입니다.
  • 모달이 열릴 때, focus-visible 스타일만 적용해서는 포커스 링이 보이지 않아 임시로 모달 내부에선 버튼 컴포넌트에 focus 스타일을 부여하는 방식으로 구현했습니다. data attributes를 통해 구현했는데, 썩 마음에 드는 방식은 아니라서 피드백 주시면 감사하겠습니다.

Breaking change or not (Yes/No)

No

References

없음

@sungik-choi sungik-choi added the enhancement Issues or PR related to making existing features better label Feb 21, 2023
@sungik-choi sungik-choi self-assigned this Feb 21, 2023
@changeset-bot
Copy link

changeset-bot bot commented Feb 21, 2023

🦋 Changeset detected

Latest commit: 6ed33c6

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

@codecov
Copy link

codecov bot commented Feb 21, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (85d04e7) 83.60% compared to head (6ed33c6) 83.60%.

Additional details and impacted files
@@           Coverage Diff            @@
##           next-v1    #1139   +/-   ##
========================================
  Coverage    83.60%   83.60%           
========================================
  Files          289      289           
  Lines         3709     3709           
  Branches       769      769           
========================================
  Hits          3101     3101           
  Misses         533      533           
  Partials        75       75           
Impacted Files Coverage Δ
...ezier-react/src/components/Button/Button.styled.ts 95.94% <ø> (ø)
...ages/bezier-react/src/components/Button/Button.tsx 100.00% <ø> (ø)
...-react/src/components/Modals/Modal/Modal.styled.ts 100.00% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 21, 2023

Chromatic Report

🚀 Congratulations! Your build was successful!

@sungik-choi sungik-choi force-pushed the feat/button-focused-style branch from 5aabbcf to 36a7660 Compare February 28, 2023 05:50
@sungik-choi sungik-choi changed the title Feat/button focused style Add focus style to Button component Mar 2, 2023
@sungik-choi sungik-choi changed the title Add focus style to Button component Add focused style to Button component Mar 2, 2023
@sungik-choi sungik-choi marked this pull request as ready for review March 2, 2023 07:57
@Dogdriip
Copy link
Contributor

Dogdriip commented Mar 2, 2023

Kapture.2023-03-02.at.18.05.06.mp4

Chromatic에서 확인해 보았는데, 마우스로 focus 시에도 해당 스타일이 보이는 것 같은데 혹시 의도하신 바가 맞을까요?

@sungik-choi
Copy link
Contributor Author

Chromatic에서 확인해 보았는데, 마우스로 focus 시에도 해당 스타일이 보이는 것 같은데 혹시 의도하신 바가 맞을까요?

넵 모달 내부에서는 맞습니다! 의도는 '모달이 열릴 때 버튼에 포커스 디자인이 보여야 한다' 였는데, focus 수도 클래스를 사용하는 방법말고는 뾰족한 수가 없다고 판단했습니다. 따라서 모달 내부에서는 마우스 focus시에도 포커스 디자인이 보이게 됩니다.

Copy link
Contributor

@Dogdriip Dogdriip left a comment

Choose a reason for hiding this comment

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

Kapture.2023-03-03.at.14.28.02.mp4

(기존 동작) open 버튼에 focus를 준 채로 마우스/키보드를 통해 열면 Cancel 버튼에 focus ring이 생기고, 버튼에 focus가 없었으면 focus ring이 생기지 않네요. 레퍼런스를 찾아봐도 이게 맞는 동작인 것 같고, 지금처럼 모달 open 시 무조건 focus ring이 생기게 하려면 이 방법밖에는 없을 것 같습니다 😓

추가로 element.focus 메소드에 focusVisible 옵션을 주면 programmatic하게 focus-visible을 줄 수 있는 것 같은데, Radix 내부 구현이 그렇게 되어 있지 않아서 나중에 생각해보면 좋을 것 같습니다.

@sungik-choi
Copy link
Contributor Author

(기존 동작) open 버튼에 focus를 준 채로 마우스/키보드를 통해 열면 Cancel 버튼에 focus ring이 생기고, 버튼에 focus가 없었으면 focus ring이 생기지 않네요. 레퍼런스를 찾아봐도 이게 맞는 동작인 것 같고, 지금처럼 모달 open 시 무조건 focus ring이 생기게 하려면 이 방법밖에는 없을 것 같습니다 😓

추가로 element.focus 메소드에 focusVisible 옵션을 주면 programmatic하게 focus-visible을 줄 수 있는 것 같은데, Radix 내부 구현이 그렇게 되어 있지 않아서 나중에 생각해보면 좋을 것 같습니다.

자세한 코멘트 감사합니다. 저도 focusVisible 옵션을 테스트해봤는데, 아직 experimental이라 그런지 잘 동작하지 않았어요

@sungik-choi sungik-choi merged commit ff32396 into channel-io:next-v1 Mar 7, 2023
@sungik-choi sungik-choi deleted the feat/button-focused-style branch March 7, 2023 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Issues or PR related to making existing features better
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants