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

Remove default value of event handler props in Icon component #1122

Merged

Conversation

heech1013
Copy link
Contributor

@heech1013 heech1013 commented Feb 6, 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

#1123

Summary

Icon 컴포넌트의 이벤트 핸들러 props에 default value를 할당하지 않도록 수정합니다.

  • onClick
  • onMouseDown

Details

IcononClick prop에 default value(noop, () => {})가 할당됨으로 인해 문제를 일으키는 경우가 있습니다.
아래의 조건을 만족할 경우 a 태그의 telephone number link(tel:)가 제대로 동작하지 않습니다.

  1. iPad의 Safari 브라우저일 때
  2. a 태그의 하위 엘리먼트 중 onclick attribute가 등록된 엘리먼트가 존재할 때

예를 들어 아래와 같은 usecase가 있습니다.

<a href="tel:+49.157.0156">
    <div onclick="doSomething()"></div>
</a>

Icon을 활용하여 tel: link로 연결되는 버튼/링크를 만드는 경우 아래와 같은 구조를 가질 수 있습니다.

<a href={telHref} target="_blank">
    <Icon source={CallIcon} size={IconSize.S} />
</a>

이때, iPad의 Safari 브라우저에서는 해당 링크가 동작하지 않게 됩니다.

  • a 태그와 Icon 사이에 다른 엘리먼트를 중첩하는 경우에도 여전히 동작하지 않습니다.
  • a 태그의 target_self, _parent, _top 등으로 변경해도 동작하지 않습니다.
  • Icon이 내부적으로 svg 태그에 전달하고 있는 onClick prop을 제거할 경우 정상 동작합니다.

--
참고: a 태그 구현의 파편화

각 환경의 브라우저마다 a 태그의 구현이 제각기 다릅니다. 특히, a 태그의 target attribute에 대해 여러 동작을 가집니다.
예를 들어, 아래와 같은 케이스가 존재합니다.

  • iOS의 Chrome 앱에서는 a 태그의 target_blank일 때 tel: link가 동작하지 않습니다. (다른 환경에서는 정상 동작)
  • iOS의 Safari 앱에서는 a 태그의 target_self일 때 tel: link가 동작하지 않습니다. (다른 환경에서는 정상 동작)

--
Workaround

아래와 같은 방법으로 Bezier Icon 컴포넌트의 수정 없이 문제를 해결할 수 있습니다.

<a
  onClick={() => window.open(`tel:${telLink}`, '_blank')}
>
    <Icon source={CallIcon} size={IconSize.S} />
</a>

단, 위와 같은 방법은 실행 환경에 따른 분기 처리를 복잡하게 만들고, iPad의 Safari 브라우저에 a 태그의 tel: link와 관련된 이슈가 있음을 개발자가 미리 알아채고 대응하기 어렵기 때문에 Bezier Icon의 수정을 통해 문제를 해결하기를 바랍니다.

Breaking change or not (Yes/No)

No

References

.

@changeset-bot
Copy link

changeset-bot bot commented Feb 6, 2023

🦋 Changeset detected

Latest commit: 10a77f4

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

@heech1013 heech1013 self-assigned this Feb 6, 2023
@heech1013 heech1013 added the fix PR related to bug fix label Feb 6, 2023
@codecov
Copy link

codecov bot commented Feb 6, 2023

Codecov Report

Base: 72.77% // Head: 72.75% // Decreases project coverage by -0.02% ⚠️

Coverage data is based on head (10a77f4) compared to base (70efd99).
Patch has no changes to coverable lines.

Additional details and impacted files
@@             Coverage Diff             @@
##           next-v1    #1122      +/-   ##
===========================================
- Coverage    72.77%   72.75%   -0.02%     
===========================================
  Files          362      362              
  Lines         4374     4371       -3     
  Branches       853      851       -2     
===========================================
- Hits          3183     3180       -3     
  Misses        1117     1117              
  Partials        74       74              
Impacted Files Coverage Δ
packages/bezier-react/src/components/Icon/Icon.tsx 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 6, 2023

Chromatic Report

🚀 Congratulations! Your build was successful!

@heech1013 heech1013 marked this pull request as ready for review February 6, 2023 07:18
@heech1013 heech1013 requested a review from dinohan February 6, 2023 07:18
@dinohan
Copy link
Member

dinohan commented Feb 6, 2023

#1079 와 컨플릭이 있겠네용

@dinohan dinohan requested a review from annie1229 February 6, 2023 07:20
Copy link
Member

@dinohan dinohan left a comment

Choose a reason for hiding this comment

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

changeset 추가해야겠네요!

@heech1013 heech1013 force-pushed the fix/icon-default-event-handler branch from 8d7dd5a to 10a77f4 Compare February 9, 2023 09:04
@heech1013 heech1013 merged commit 48f6a3b into channel-io:next-v1 Feb 9, 2023
sungik-choi added a commit that referenced this pull request Feb 7, 2024
<!--
  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 #1122

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

Remove default value of event handler props

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

#1122 를 참고해주세요.

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

No
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