Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary of ChangesHello @Greensod-96, 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! ์ด PR์ ํ ์คํธ ์๋ํฐ์ ํฐํธ ๋ฐ ๋ฆฌ์คํธ ์ฒ๋ฆฌ ๊ธฐ๋ฅ์ ๊ฐ์ ํ๊ณ , ๋ชจ๋ฌ ์ปดํฌ๋ํธ์ ์ ๋๋ฉ์ด์ ์ ์ถ๊ฐํ์ฌ ์ฌ์ฉ์ ๊ฒฝํ์ ํฅ์์ํค๋ ๋ฐ ์ค์ ์ ๋ก๋๋ค. ๋ํ, ๋ฉ์์ง ์์ฑ ํ์ด์ง์ ํ๋กํ ์ด๋ฏธ์ง ์ ํ ๊ณผ์ ์ ๋ ์ง๊ด์ ์ผ๋ก ๋ง๋ค๊ณ , Quill ์๋ํฐ ์ฝํ ์ธ ์ ์์ ํ ๋ ๋๋ง์ ์ํ ์ค์ ์ ์ ๋ฐ์ดํธํ์ต๋๋ค. 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.
Code Review
This pull request addresses a bug related to text editor functionalities and modal display issues. The changes include enhancements to font application, list formatting in the text editor, and modal transition effects. Additionally, profile image handling in the message posting form has been improved. The review focuses on correctness and maintainability, particularly in the context of React component lifecycle management and DOM manipulation.
| // ๋ซํ ๋ ์ ๋๋ฉ์ด์ ๊ณ ๋ คํด ์ฝ๊ฐ์ ์ง์ฐ | ||
| const timer = setTimeout(() => setVisible(false), 200); | ||
| return () => clearTimeout(timer); |
There was a problem hiding this comment.
๋ชจ๋ฌ์ด ๋ซํ ๋ setTimeout์ ์ฌ์ฉํ๋ ๊ฒ์ ์ ๋๋ฉ์ด์
์ด ์๋ฃ๋๊ธฐ ์ ์ Portal์ด ์ ๊ฑฐ๋ ์ ์๋ ์ ์ฌ์ ์ธ ๋ฌธ์ ๋ฅผ ์ผ๊ธฐํ ์ ์์ต๋๋ค. setVisible(false)๋ฅผ ํธ์ถํ๊ธฐ ์ ์ ์ ๋๋ฉ์ด์
์ด ์๋ฃ๋์๋์ง ํ์ธํ๋ ๊ฒ์ด ์ข์ต๋๋ค. CSS ์ ๋๋ฉ์ด์
์ด๋ฒคํธ๋ฅผ ์ฌ์ฉํ์ฌ ์ด๋ฅผ ๋๊ธฐํํ ์ ์์ต๋๋ค.
Using setTimeout when the modal closes might cause a potential issue where the Portal is removed before the animation completes. It's recommended to ensure the animation is complete before calling setVisible(false). You could use CSS animation events to synchronize this.
| onFontChange={(fontKey) => { | ||
| if (!fontKey) { | ||
| return; | ||
| } | ||
| setSelectedFont(fontKey); | ||
|
|
||
| const displayName = FONT_DISPLAY_NAMES[fontKey]; | ||
| if (displayName && fontRef.current?.setValue) { | ||
| fontRef.current.setValue(displayName); | ||
| } | ||
| }} |
There was a problem hiding this comment.
onFontChange prop์ ํตํด ์ ๋ฌ๋๋ ์ฝ๋ฐฑ ํจ์ ๋ด์์ fontRef.current?.setValue๋ฅผ ํธ์ถํ๋ ๊ฒ์ side effect๋ฅผ ๋ฐ์์ํฌ ์ ์์ต๋๋ค. ์ปดํฌ๋ํธ์ ์ํ ์
๋ฐ์ดํธ๋ ์์ธก ๊ฐ๋ฅํด์ผ ํ๋ฉฐ, ๋ถ๋ชจ ์ปดํฌ๋ํธ์ ์ํ๋ฅผ ์์ ์ปดํฌ๋ํธ์์ ์ง์ ๋ณ๊ฒฝํ๋ ๊ฒ์ ๊ถ์ฅ๋์ง ์์ต๋๋ค. ๋์ , ๋ถ๋ชจ ์ปดํฌ๋ํธ์์ ์ํ๋ฅผ ์
๋ฐ์ดํธํ๊ณ , ํ์ํ ๊ฒฝ์ฐ ์์ ์ปดํฌ๋ํธ์ prop์ผ๋ก ์ ๋ฌํ๋ ๊ฒ์ด ์ข์ต๋๋ค.
Calling fontRef.current?.setValue within the callback function passed through the onFontChange prop can cause side effects. Component state updates should be predictable, and directly modifying the parent component's state from a child component is not recommended. Instead, it's better to update the state in the parent component and pass it to the child component as a prop if necessary.
| }, [font]); | ||
|
|
||
| // Quill์์ ๊ธ๊ผด ๋ณ๊ฒฝ ๊ฐ์ง โ ์ธ๋ถ๋ก ์ ๋ฌ (์ ํ ์์ญ ๊ธฐ์ค) | ||
| // Quill์์ ๊ธ๊ผด ๋ณ๊ฒฝ ๊ฐ์ง โ ์ธ๋ถ๋ก ์ ๋ฌ (ํด๋ฐ ์ฌ์ฉ ์๋ง) |
There was a problem hiding this comment.
// Quill์์ ๊ธ๊ผด ๋ณ๊ฒฝ ๊ฐ์ง โ ์ธ๋ถ๋ก ์ ๋ฌ (ํด๋ฐ ์ฌ์ฉ ์๋ง) ์ฃผ์์ ๋ณ๊ฒฝ๋ ๋ก์ง์ ์ ํํ ๋ฐ์ํ์ง ์์ต๋๋ค. ํด๋ฐ๋ฅผ ์ฌ์ฉํ์ง ์๊ณ ๋ ๊ธ๊ผด์ด ๋ณ๊ฒฝ๋ ์ ์์ต๋๋ค. ์ฃผ์์ ์
๋ฐ์ดํธํ์ฌ ๋ณ๊ฒฝ ์ฌํญ์ ๋ ์ ํํ๊ฒ ์ค๋ช
ํ๋ ๊ฒ์ด ์ข์ต๋๋ค.
Consider updating the comment to accurately reflect the change in logic.
| if (format.font && format.font !== lastSelectionFontRef.current) { | ||
| lastSelectionFontRef.current = format.font; | ||
| onFontChange(format.font); | ||
| } |
There was a problem hiding this comment.
format.font !== lastSelectionFontRef.current ๋น๊ต๋ ๋ถํ์ํด ๋ณด์
๋๋ค. onFontChange ์ฝ๋ฐฑ์ ํญ์ ์ธ๋ถ์์ ์ ํ๋ ๊ธ๊ผด์ ๊ธฐ๋ฐ์ผ๋ก ํธ์ถ๋๋ฏ๋ก, ์ด ์กฐ๊ฑด์ ํญ์ ์ฐธ์ด ๋ ๊ฒ์
๋๋ค. ์ด ์กฐ๊ฑด์ ์ ๊ฑฐํ์ฌ ์ฝ๋๋ฅผ ๋จ์ํํ๋ ๊ฒ์ด ์ข์ต๋๋ค.
Consider removing the condition format.font !== lastSelectionFontRef.current to simplify the code.
onFontChange(format.font);| // ๐จ 1, 4๋ฒ ๋ฌธ์ ํด๊ฒฐ: ๋ชจ๋ ๋ฆฌ์คํธ ํญ๋ชฉ(li)์ ๊ธฐ์ค์ ๊ณผ ํ ์คํธ ์์ ์์น ์ค์ | ||
| const listItems = container.querySelectorAll('li'); | ||
| listItems.forEach((li) => { | ||
| // ql-ui์ absolute ๊ธฐ์ค์ | ||
| li.style.position = 'relative'; | ||
| // ํ ์คํธ๊ฐ ๊ฒน์น์ง ์๋๋ก ์ค๋ฅธ์ชฝ์ผ๋ก ๋ฐ์ด๋ | ||
| li.style.paddingLeft = '1.5em'; | ||
| }); |
There was a problem hiding this comment.
li ์์์ ์คํ์ผ์ ์ง์ ์ ์ฉํ๋ ๊ฒ์ ์ ์ง๋ณด์๋ฅผ ์ด๋ ต๊ฒ ๋ง๋ค ์ ์์ต๋๋ค. CSS ํด๋์ค๋ฅผ ์ฌ์ฉํ๊ณ , ํ์ํ ๊ฒฝ์ฐ ํด๋น ํด๋์ค๋ฅผ ๋์ ์ผ๋ก ์ถ๊ฐ/์ ๊ฑฐํ๋ ๊ฒ์ด ์ข์ต๋๋ค. ์ด๋ ๊ฒ ํ๋ฉด ์คํ์ผ ๋ณ๊ฒฝ์ด ๋ ์ฝ๊ณ ์์ธก ๊ฐ๋ฅํด์ง๋๋ค.
Applying styles directly to li elements can make maintenance difficult. It's recommended to use CSS classes and dynamically add/remove those classes as needed. This makes style changes easier and more predictable.
| // ์ด๋ฏธ ์์ฑ๋ ๋ง์ปค๊ฐ ์๋ค๋ฉด ๊ฑด๋๋๋๋ค. (์์ ์ฅ์น) | ||
| if (li.querySelector('.ql-ui')) { | ||
| return; |
There was a problem hiding this comment.
์ด๋ฏธ ์์ฑ๋ ๋ง์ปค๊ฐ ์๋์ง ํ์ธํ๋ ๋ก์ง(li.querySelector('.ql-ui'))์ ์ค๋ณต ์์ฑ์ ๋ฐฉ์งํ๋ ๋ฐ ์ ์ฉํ์ง๋ง, ๋ง์ปค๊ฐ ์์์น ์๊ฒ ์ ๊ฑฐ๋ ๊ฒฝ์ฐ๋ฅผ ์ฒ๋ฆฌํ์ง ์์ต๋๋ค. ๋ง์ปค๊ฐ ์กด์ฌํ์ง ์์ ๋๋ง๋ค ๋ง์ปค๋ฅผ ๋ค์ ์์ฑํ๋๋ก ๋ก์ง์ ์กฐ์ ํ๋ ๊ฒ์ด ์ข์ต๋๋ค.
The logic to check if a marker already exists (li.querySelector('.ql-ui')) is useful for preventing duplicate creation, but it doesn't handle cases where the marker might have been unexpectedly removed. Consider adjusting the logic to recreate the marker whenever it's not present.
| <PostContent | ||
| htmlContent={content} | ||
| className={'line-clamp-2 sm:line-clamp-3'} | ||
| font={font} | ||
| card |
There was a problem hiding this comment.
PostContent ์ปดํฌ๋ํธ๋ฅผ ์ฌ์ฉํ์ฌ ๋ฉ์์ง ๋ด์ฉ์ ํ์ํ๋ ๊ฒ์ ์ข์ง๋ง, line-clamp ํด๋์ค๋ฅผ PostContent ๋ด๋ถ๋ก ์ด๋์ํค๋ ๊ฒ์ ๊ณ ๋ คํด ๋ณด์ธ์. ์ด๋ ๊ฒ ํ๋ฉด MessageCard ์ปดํฌ๋ํธ๊ฐ ๋ ๊น๋ํด์ง๊ณ , PostContent๊ฐ ์์ฒด์ ์ผ๋ก ํ
์คํธ ์ค truncation์ ์ฒ๋ฆฌํ๋๋ก ํ ์ ์์ต๋๋ค.
While using the PostContent component to display the message content is good, consider moving the line-clamp class inside PostContent. This would make the MessageCard component cleaner and allow PostContent to handle text truncation on its own.
| !fromInput.value?.trim() || | ||
| !profileImageURL?.trim() || |
There was a problem hiding this comment.
fromInput.value?.trim() ๋ฐ profileImageURL?.trim()์ ๋ํ nullish coalescing ์ฐ์ฐ์๋ฅผ ์ฌ์ฉํ๋ ๊ฒ์ ์ข์ต๋๋ค. ์ด๋ ๊ฒ ํ๋ฉด ๊ฐ์ด null ๋๋ undefined์ธ ๊ฒฝ์ฐ ์ค๋ฅ๊ฐ ๋ฐ์ํ๋ ๊ฒ์ ๋ฐฉ์งํ ์ ์์ต๋๋ค.
Using nullish coalescing operators for fromInput.value?.trim() and profileImageURL?.trim() is good. This prevents errors if the values are null or undefined.
|
|
||
| .ql-snow.ql-toolbar button.ql-list.ql-active .ql-stroke, | ||
| .ql-snow.ql-toolbar button.ql-list.ql-active .ql-stroke, | ||
| .ql-snow .ql-toolbar button.ql-list.ql-active .ql-stroke, |
There was a problem hiding this comment.
.ql-snow .ql-toolbar button.ql-list.ql-active .ql-stroke์ ๋ํ ์ค๋ณต ์ ์ธ์ด ์์ต๋๋ค. ์ด ์ค๋ณต์ ์ ๊ฑฐํ์ฌ CSS๋ฅผ ์ ๋ฆฌํ๋ ๊ฒ์ด ์ข์ต๋๋ค.
There is a duplicate declaration for .ql-snow .ql-toolbar button.ql-list.ql-active .ql-stroke. Consider removing this duplication to clean up the CSS.
|
๐ ๊ตฌํํ ๊ธฐ๋ฅ Preview: https://frontend-nbq9ooj79-fe-19-seongihuns-projects.vercel.app |
โฆprint-19-Rolling/frontend into fix/profile-image-basic-Delete
|
๐ ๊ตฌํํ ๊ธฐ๋ฅ Preview: https://frontend-dyx5cbheg-fe-19-seongihuns-projects.vercel.app |
โฆprint-19-Rolling/frontend into fix/profile-image-basic-Delete
|
๐ ๊ตฌํํ ๊ธฐ๋ฅ Preview: https://frontend-5jqut8t6d-fe-19-seongihuns-projects.vercel.app |
|
๐ ๊ตฌํํ ๊ธฐ๋ฅ Preview: https://frontend-4xcca8bbr-fe-19-seongihuns-projects.vercel.app |
aahreum
left a comment
There was a problem hiding this comment.
๊ณ ์ํ์ จ์ต๋๋ค!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!1
๐ ์ด์ ๋ฒํธ
โจ ์์ ํ ๋ด์ฉ
๐ Review Point
๐ก ์ฐธ๊ณ ์ฌํญ