Skip to content

Conversation

@sungik-choi
Copy link
Contributor

@sungik-choi sungik-choi commented Nov 30, 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.
  • [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

Fixes #978

Summary

ConfirmModal 을 재구현합니다.

Details

#1036을 베이스로, 해당 PR에서 구현한 Modal 컴포넌트를 바탕으로 구현합니다. 초기엔 Radix UI의 AlertDialog를 사용하여 구현을 시도했으나, 몇가지 불편한 점이 있었습니다.

  • Dialog 와 다르게, AlertDialog 에선 모달 바깥의 오버레이를 클릭해도 모달이 닫히지 않도록 구현되어 있습니다. WAI - Alert Dialog의 패턴을 확인해봐도 오버레이에 대한 동작을 강제하는 내용은 없기에 굳이 이 UX를 따라야할 필요는 없다고 생각했습니다.
  • Dialog 의 모달이 열릴 경우 모달 내부의 포커스 가능한 첫번째 요소에 포커싱하는 기능이 없고, 대신 AlertDialogCancel 이라는 컴포넌트를 통해 개발자가 수동으로 포커싱되어야하는 '취소' 버튼 컴포넌트를 지정해줘야 하는 불편함이 있었습니다. WAI의 예시를 보면,

Focus is automatically set to the first focusable element inside the dialog, which is the "No" button. This is the least destructive action, so focusing "No" helps prevent users from accidentally confirming the destructive "Discard" action, which cannot be undone.

이라는 문구가 있으나, 현재 채널 데스크 애플리케이션의 디자인대로라면 1. 폼 요소등이 모달에 포함되어있을 경우 (예 - 삭제 하기 전 DELETE 문자 입력 폼) 이 폼에 먼저 포커싱이 가야 하며(the first focusable element inside the dialog), 2. 굳이 수동으로 지정할 필요 없이 모달의 버튼 순서(ButtonGroup)가 취소 - 액션 순으로 되어있으므로 자동으로 취소 버튼에 포커싱이 갈 거로 생각했습니다. AlertDialog 를 그대로 사용하면 이 동작을 사용할 수 없었습니다.

  • Radix Alert Dialog의 구현을 살펴보면, 사실상 Dialog 를 내부 구현에 그대로 사용하고 있습니다. 다른 점은 상술한 점들과 role="alertdialog" 라는 점 정도라, 중복 구현없이 충분히 구현할 수 있겠다고 생각했습니다.

Breaking change or not (Yes/No)

Yes

  • onConfirm : 제거합니다. 엔터 키를 누르면 호출되는 핸들러인데, 엔터 키는 포커싱된 버튼을 클릭하는 용도로 사용되어야하지, 강제로 액션을 발생시키는 용도로 사용해선 안됩니다.
  • ConfirmModalHeader : ModalHeadershowCloseIcon prop을 제거합니다. 시안상 컨펌 모달의 역할을 하는 UI의 경우 대부분 우측 상단 X 버튼이 없기에 굳이 사용자에게 노출할 인터페이스가 아니라고 판단했습니다.
  • 그 외 다른 브레이킹 체인지는 Re-implement Modal component using Dialog in Radix UI #1036 과 같습니다.

References

@sungik-choi sungik-choi added enhancement Issues or PR related to making existing features better component a11y Issue or PR related to accessibility labels Nov 30, 2022
@sungik-choi sungik-choi self-assigned this Nov 30, 2022
@changeset-bot
Copy link

changeset-bot bot commented Nov 30, 2022

🦋 Changeset detected

Latest commit: e1def2f

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 Minor
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

@sungik-choi sungik-choi changed the title [WIP] Re-implement ConfirmModal component using AlertDialog in Radix UI [WIP] Re-implement ConfirmModal component using AlertDialog in Radix UI Nov 30, 2022
@codecov
Copy link

codecov bot commented Nov 30, 2022

Codecov Report

Base: 71.72% // Head: 71.79% // Increases project coverage by +0.06% 🎉

Coverage data is based on head (e1def2f) compared to base (e23c54a).
Patch coverage: 50.00% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff             @@
##           next-v1    #1050      +/-   ##
===========================================
+ Coverage    71.72%   71.79%   +0.06%     
===========================================
  Files          217      223       +6     
  Lines         3056     3067      +11     
  Branches       842      842              
===========================================
+ Hits          2192     2202      +10     
- Misses         740      741       +1     
  Partials       124      124              
Impacted Files Coverage Δ
...rc/components/Modals/ConfirmModal/ConfirmModal.tsx 100.00% <ø> (+100.00%) ⬆️
...omponents/Modals/ConfirmModal/ConfirmModalBody.tsx 0.00% <0.00%> (ø)
...ponents/Modals/LegacyConfirmModal/ConfirmModal.tsx 0.00% <0.00%> (ø)
...onents/Modals/ConfirmModal/ConfirmModalContent.tsx 100.00% <100.00%> (ø)
...ponents/Modals/ConfirmModal/ConfirmModalFooter.tsx 100.00% <100.00%> (ø)
...ponents/Modals/ConfirmModal/ConfirmModalHeader.tsx 100.00% <100.00%> (ø)
...onents/Modals/ConfirmModal/ConfirmModalHelpers.tsx 100.00% <100.00%> (ø)
...-react/src/components/Modals/Modal/ModalHeader.tsx 84.21% <0.00%> (+5.26%) ⬆️

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.

@sungik-choi sungik-choi marked this pull request as draft December 1, 2022 07:50
@sungik-choi sungik-choi force-pushed the feat/implement-confirm-modal branch from 4b9eeb8 to 349f021 Compare December 2, 2022 10:58
@sungik-choi sungik-choi changed the title [WIP] Re-implement ConfirmModal component using AlertDialog in Radix UI [WIP] Re-implement ConfirmModal component using Modal Dec 2, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Dec 2, 2022

Chromatic Report

🚀 Congratulations! Your build was successful!

@sungik-choi sungik-choi changed the title [WIP] Re-implement ConfirmModal component using Modal Re-implement ConfirmModal component using Modal Dec 2, 2022
@sungik-choi sungik-choi marked this pull request as ready for review December 2, 2022 11:21
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 sungik-choi force-pushed the feat/implement-confirm-modal branch from 349f021 to cdd2017 Compare December 6, 2022 11:22
@sungik-choi sungik-choi requested a review from quino0627 December 6, 2022 11:23
@yangwooseong
Copy link
Contributor

레거시 모달을 전부 지우셨는데, 일단 남겨두고 데스크에서 모달 마이그레이션이 모두 진행되고 버그가 없는 것을 확인한 다음에 지우는 것은 어떻게 생각하시는지 궁금합니다! 데스크에서 모달이 굉장히 많아서, 혹시라도 예상히 못한 버그가 발생했을 때 대응이 어려워질 것 같은 느낌도 들어서요

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.

@yangwooseong 말씀대로 점진적 마이그레이션도 생각해볼만할 것 같습니다. LegacyModal을 LEGACY__Modal 정도로 이름을 바꿔서 export하고 사용처에서 우선 import 이름만 전부 바꾼 다음, 점진적으로 마이그레이션하는 것도 괜찮을 것 같습니다.

코드 자체에는 이견이 없어서 Approve 남겼습니다!

}: ConfirmModalContentProps, forwardedRef: React.Ref<HTMLDivElement>) {
return (
<ModalContent
role="alertdialog"
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@sungik-choi
Copy link
Contributor Author

레거시 모달을 전부 지우셨는데, 일단 남겨두고 데스크에서 모달 마이그레이션이 모두 진행되고 버그가 없는 것을 확인한 다음에 지우는 것은 어떻게 생각하시는지 궁금합니다! 데스크에서 모달이 굉장히 많아서, 혹시라도 예상히 못한 버그가 발생했을 때 대응이 어려워질 것 같은 느낌도 들어서요

큰 문제 없을거라고 생각해서 제거했는데, 만약을 대비하여 남겨두는 편이 더 좋겠네요

@sungik-choi sungik-choi force-pushed the feat/implement-confirm-modal branch from cdd2017 to 8e1129e Compare December 7, 2022 13:17
@sungik-choi sungik-choi merged commit 052fdb9 into channel-io:next-v1 Dec 13, 2022
@sungik-choi sungik-choi deleted the feat/implement-confirm-modal branch December 13, 2022 04:24
@sungik-choi sungik-choi added the #reimplementation Issue or PR related to Reimplementation of existing components (#1105) label Jan 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a11y Issue or PR related to accessibility enhancement Issues or PR related to making existing features better #reimplementation Issue or PR related to Reimplementation of existing components (#1105)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Re-implement Modal, ConfirmModal component using Dialog, AlertDialog in Radix UI

4 participants