-
Notifications
You must be signed in to change notification settings - Fork 10
fix: prevent to trigger event from svg of icon-button #345
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
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary of ChangesHello @noahchoii, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a critical bug in the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
useMemo를 사용하여 불필요한 리렌더링을 방지한 수정은 올바르며, 문제의 원인을 정확히 해결합니다. 관련 테스트 케이스를 추가하여 회귀를 방지한 점도 좋습니다. 다만, 추가된 테스트 코드의 명확성과 일관성을 높이기 위한 몇 가지 제안 사항이 있습니다.
| </IconButton> | ||
| ); | ||
| }; | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
|
||
| const icon2 = rendered.getByTestId('icon'); | ||
|
|
||
| expect(icon).toBe(icon2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
✅ All tests passed!
Click here if you need to update snapshots. |
MaxLee-dev
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Icon-button is solved like this, but the same phenomenon is likely to occur in the part where createSlot is used. So I think it was modified by this PR , so it would be good to refer to it.
|
@MaxLee-dev I agree. However, I will merge this PR first to resolve the internal CS issue! |
Description of Changes
Cause
createSlotcreates a new component on every render, causing React to recognize it as a type change and remount the DOM.Subsequently, during click event handling, the remount occurs, removing the event target from the DOM and halting event propagation.
Event flow:
IconButtonre-renders →createSlotre-calledSVG DOM removed → Event propagation interrupted 💥
Solution
Use
useMemofor component memoization: Reuse cached components when children references are identical to prevent unnecessary remounts.Checklist
Before submitting the PR, please make sure you have checked all of the following items.