구독 모달의 구조를 카테고리 중심에서 키워드 중심으로 변경 #109
Conversation
WalkthroughThis update adds the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SubscriptionModalContent
participant useCheckEmail
participant API
User->>SubscriptionModalContent: Open modal
SubscriptionModalContent->>User: Show keyword chips, email input
User->>SubscriptionModalContent: Enter email, click "인증번호 받기"
SubscriptionModalContent->>useCheckEmail: Validate email
useCheckEmail-->>SubscriptionModalContent: Return validity
alt Email valid
SubscriptionModalContent->>API: Send verification email
API-->>SubscriptionModalContent: Response (logged)
end
User->>SubscriptionModalContent: Enter verification code (input disabled until sent)
User->>SubscriptionModalContent: Click "구독 완료" (button disabled until verified and consent given)
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
src/components/common/chip.tsx (1)
18-38: CSS properties might be conflictingThe combination of
width: 100%withmax-width: fit-content(and similar for height) might be confusing or potentially conflicting.Consider simplifying the size properties:
const chipCss = css` display: flex; align-items: center; column-gap: 2px; background-color: #fff; border: 1px solid var(--gray-10); border-radius: 4px; padding: 4px 8px; - width: 100%; - height: 100%; - max-width: fit-content; - max-height: fit-content; + width: fit-content; + height: fit-content; & > img { cursor: pointer; } & > span { color: var(--text-body2); } `;src/components/app/signup/certificate-box.tsx (2)
18-23: Incorrectidvalue and potential accessibility issueThe JSX element is given an
id="certificateb-box"(note the extra b) which is likely a typo.
While IDs are rarely consumed directly in React, typos make DOM inspection/debugging harder and can break automated tests or CSS hooks relying on the correct value.Suggested fix:
- id="certificateb-box" + id="certificate-box"Also applies to: 24-31
33-42:cursor: pointerremains active even when the component is disabledAlthough
pointer-events: noneprevents clicks, the cursor styling still communicates that the element is interactive.
To give consistent feedback to users (and assistive tech), switch the cursor todefaultin the disabled branch.- pointer-events: none; - background-color: var(--gray-10); - color: var(--text-disabled); + pointer-events: none; + cursor: default; + background-color: var(--gray-10); + color: var(--text-disabled);src/components/common/modal/SubscriptionModalContent.tsx (1)
22-33: Unused variableres– remove or log meaningfullyEven after awaiting,
resis never used.
Either destructure needed data (e.g., HTTP status) or omit the variable:- const res = await sendVerifyEmail({...}); - setVerifySendCheck(true); + await sendVerifyEmail({...}); + setVerifySendCheck(true);Also applies to: 36-47
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlsrc/assets/chip/delete.svgis excluded by!**/*.svg
📒 Files selected for processing (11)
package.json(1 hunks)src/App.tsx(1 hunks)src/app/home/index.tsx(1 hunks)src/components/app/signup/certificate-box.tsx(1 hunks)src/components/common/Button.tsx(3 hunks)src/components/common/Input.tsx(4 hunks)src/components/common/chip.tsx(1 hunks)src/components/common/modal/Modal.tsx(2 hunks)src/components/common/modal/SubscriptionModalContent.tsx(3 hunks)src/style/variable.scss(1 hunks)src/types/modal.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/app/home/index.tsx (1)
src/components/common/modal/SubscriptionModalContent.tsx (1)
SubscriptionModalContent(14-124)
src/components/app/signup/certificate-box.tsx (1)
src/style/variable.ts (1)
DESIGN_SYSTEM_TEXT(3-121)
src/components/common/modal/SubscriptionModalContent.tsx (9)
src/hooks/useModal.ts (1)
useModal(9-31)src/hooks/useCheckEmail.ts (1)
useCheckEmail(4-19)src/hooks/api/member/useApiSendVerifyEmail.ts (1)
sendVerifyEmail(7-14)src/utils/common.ts (1)
EMAIL_PURPOSE(11-11)src/hooks/api/member/useApiVerifyEmail.ts (1)
verifyEmail(8-16)src/components/common/chip.tsx (1)
Chip(9-16)src/components/common/Input.tsx (1)
Input(8-63)src/components/app/signup/certificate-box.tsx (1)
CertificateBox(11-49)src/components/common/Button.tsx (1)
Button(9-54)
🪛 GitHub Check: build
src/components/common/modal/SubscriptionModalContent.tsx
[failure] 39-39:
'res' is declared but its value is never read.
[failure] 36-36:
'handleVerifyEmailCode' is declared but its value is never read.
[failure] 18-18:
'setKeywordList' is declared but its value is never read.
[failure] 18-18:
'keywordList' is declared but its value is never read.
🪛 ESLint
src/components/common/modal/SubscriptionModalContent.tsx
[error] 18-18: 'keywordList' is assigned a value but never used.
(@typescript-eslint/no-unused-vars)
[error] 18-18: 'setKeywordList' is assigned a value but never used.
(@typescript-eslint/no-unused-vars)
[error] 36-36: 'handleVerifyEmailCode' is assigned a value but never used.
(@typescript-eslint/no-unused-vars)
[error] 39-39: 'res' is assigned a value but never used.
(@typescript-eslint/no-unused-vars)
🔇 Additional comments (18)
package.json (1)
46-46: LGTM: Added Sass dependencyThe addition of the Sass dependency is appropriate for enabling SCSS styling in the project, which is used in the new variable.scss file.
src/style/variable.scss (1)
20-20: Color update for disabled text is appropriateChanging the disabled text color to a lighter gray (#c6c6c6) improves the visual distinction for disabled UI elements.
src/App.tsx (1)
6-7: LGTM: Good import order for stylesAdding the SCSS variables import is appropriate, and placing it after global.css ensures proper style cascade.
src/types/modal.ts (1)
4-4: LGTM: Making title optional provides flexibilityMaking the title property optional is a good change that provides more flexibility for different modal designs.
src/app/home/index.tsx (2)
114-117: LGTM: Consistent JSDoc formatting.The JSDoc comment formatting looks clean, maintaining a consistent style with the rest of the codebase.
120-121: Better maintainability with trailing comma.Adding the trailing comma improves maintainability by ensuring future additions to this object won't cause multi-line git diffs.
src/components/common/modal/Modal.tsx (2)
60-60: Improved visual balance with consistent padding.Changing to uniform padding (3.2rem) creates a more balanced and visually pleasing modal container, consistent with modern UI design principles.
94-94: LGTM: Consistent style formatting.The spacing around the colon and use of double quotes improves code readability while maintaining the same functionality.
src/components/common/Input.tsx (4)
16-16: Improved layout control with explicit width.Setting
width: 100%ensures the input container properly fills its parent container, preventing unexpected layout issues.
29-30: Enhanced precision with fixed pixel values.Changing from
2remto16pxprovides more precise control over spacing and aligns with the pixel-based approach used in other style updates.
39-42: UI consistency improvements with updated dimensions and styling.The changes to height, border-radius, font-size, and color create a more consistent input component that aligns with the overall design system. Using CSS variables for text color improves theme consistency.
52-56: Properly handled disabled state.Adding specific styles for the disabled state improves user experience by providing clear visual feedback when an input is not interactive. Using CSS variables ensures consistency with other disabled elements.
src/components/common/Button.tsx (6)
3-4: Good identification of future improvement.The TODO comment correctly identifies the need to separate inline styles, which will improve maintainability and reusability in the future.
9-9: Simplified component API.Removing the
stateboolean parameter streamlines the component's API by relying on the standarddisabledHTML attribute instead of a custom prop.
13-18: Improved container layout control.Setting
width: 100%ensures the button container properly fills its parent, preventing layout inconsistencies.
24-25: Enhanced precision with fixed pixel values.Changing from
3.6remto16pxprovides more precise spacing control and aligns with the pixel-based approach in other component updates.
32-40: Modernized button styling with improved visual consistency.The updates to height, border-radius, font-size, and font-weight create a more modern and consistent button appearance. Adding a transition provides a smoother user experience when states change.
42-46: Better disabled state handling with CSS pseudo-classes.Replacing the conditional
stateprop with the standard:disabledpseudo-class is a significant improvement that:
- Follows web standards
- Makes the code more maintainable
- Leverages CSS variables for consistent theming
This approach is more elegant and sustainable than the previous implementation.
klmhyeonwoo
force-pushed
the
feature/102
branch
2 times, most recently
from
9a653fc to
5d1640f
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (5)
src/components/common/modal/SubscriptionModalContent.tsx (5)
52-64: API calls need proper async/await handling.The
sendVerifyEmailAPI call is not awaited, so the try/catch block won't catch Promise rejections andsetVerifySendCheck(true)will execute regardless of success or failure.
66-78: Function is defined but never used.The
handleVerifyEmailCodefunction is declared but never called anywhere in the component.🧰 Tools
🪛 ESLint
[error] 66-66: 'handleVerifyEmailCode' is assigned a value but never used.
(@typescript-eslint/no-unused-vars)
[error] 69-69: 'res' is assigned a value but never used.
(@typescript-eslint/no-unused-vars)
86-88: Reference to removedcategoriesobject causes runtime crash.The
categoriesobject no longer exists after migrating to keyword-based selection, but it's still referenced here, which will cause aReferenceError.
161-166: Email verification input is permanently disabled.The verification code input is hard-coded as
disabled={true}, preventing users from entering the verification code.
168-170: Subscription button is permanently disabled.The subscription button is hard-coded as
disabled={true}, preventing users from completing the subscription process.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/assets/chip/plus.svgis excluded by!**/*.svg
📒 Files selected for processing (2)
src/components/common/chip.tsx(1 hunks)src/components/common/modal/SubscriptionModalContent.tsx(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/components/common/modal/SubscriptionModalContent.tsx (9)
src/hooks/useModal.ts (1)
useModal(9-31)src/hooks/useCheckEmail.ts (1)
useCheckEmail(4-19)src/hooks/api/member/useApiSendVerifyEmail.ts (1)
sendVerifyEmail(7-14)src/utils/common.ts (1)
EMAIL_PURPOSE(11-11)src/hooks/api/member/useApiVerifyEmail.ts (1)
verifyEmail(8-16)src/components/common/chip.tsx (1)
Chip(14-43)src/components/common/Input.tsx (1)
Input(8-63)src/components/app/signup/certificate-box.tsx (1)
CertificateBox(11-49)src/components/common/Button.tsx (1)
Button(9-54)
🪛 ESLint
src/components/common/modal/SubscriptionModalContent.tsx
[error] 13-13: 'set' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 66-66: 'handleVerifyEmailCode' is assigned a value but never used.
(@typescript-eslint/no-unused-vars)
[error] 69-69: 'res' is assigned a value but never used.
(@typescript-eslint/no-unused-vars)
🔇 Additional comments (2)
src/components/common/chip.tsx (1)
6-12: Type definition looks good for dual-mode component.The prop types appropriately support both inputable and display modes with optional handlers.
src/components/common/modal/SubscriptionModalContent.tsx (1)
175-274: Well-structured CSS organization.The new CSS-in-JS styling is well-organized with logical groupings and consistent use of CSS variables. The responsive design and spacing are appropriate.
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (3)
src/app/signup/index.tsx (3)
169-175:⚠️ Potential issueCritical logic error: Signup button disabled condition is inverted.
The signup button should be enabled when all required fields are filled and email is verified, but the current logic disables it when all conditions are met. This prevents users from completing signup.
Apply this fix:
- disabled={Boolean( - verifyEmailNumCheck && - email && - password && - verifyPassWordNum && - nickname, - )} + disabled={!( + verifyEmailNumCheck && + email && + password && + verifyPassWordNum && + nickname + )}
147-150: 🛠️ Refactor suggestionMissing password validation: Passwords should match.
The form collects both password and password confirmation but doesn't validate that they match before allowing submission.
Add password matching validation:
+ disabled={!( + verifyEmailNumCheck && + email && + password && + verifyPassWordNum && + password === verifyPassWordNum && + nickname + )}
132-132: 🛠️ Refactor suggestionMissing password strength validation.
The placeholder mentions specific requirements ("영문 숫자, 6-17자") but there's no validation to enforce these rules.
Consider adding password strength validation:
const isValidPassword = (pwd: string) => { const hasLetter = /[a-zA-Z]/.test(pwd); const hasNumber = /\d/.test(pwd); const isValidLength = pwd.length >= 6 && pwd.length <= 17; return hasLetter && hasNumber && isValidLength; };
♻️ Duplicate comments (3)
src/components/common/chip.tsx (3)
31-31: CSS conflict issue from previous review still present.The input element still incorrectly applies
chipCssinside aninputableChipCsscontainer, creating styling conflicts as identified in previous reviews.
104-114: Unused input-specific styles still present in chipCss.The
chipCssstill contains input-specific styles that are unnecessary and inconsistent, as noted in previous reviews. These should be removed to clean up the styling.
10-10: Confusing onClick prop usage still unresolved.The
onClickprop serves different purposes in inputable mode (add functionality) vs closable mode (delete functionality), which creates confusion as mentioned in previous reviews. Consider separating these into distinct props likeonAddandonDelete.
🧹 Nitpick comments (1)
src/components/common/modal/SubscriptionModalContent.tsx (1)
42-42: Fix variable name typo.There's a typo in the variable name
remaingTime- it should beremainingTime.- const [remaingTime, setRemainingTime] = useState(300); + const [remainingTime, setRemainingTime] = useState(300);Also update the usage on line 317:
- <span> 유효시간 {dateUtil.formatTime(remaingTime)} </span> + <span> 유효시간 {dateUtil.formatTime(remainingTime)} </span>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src/assets/checkbox/circle_checked_default.svgis excluded by!**/*.svgsrc/assets/checkbox/circle_checked_fill.svgis excluded by!**/*.svg
📒 Files selected for processing (9)
src/api/index.ts(1 hunks)src/app/signin/index.tsx(1 hunks)src/app/signup/consent/index.tsx(1 hunks)src/app/signup/index.tsx(3 hunks)src/components/common/Input.tsx(5 hunks)src/components/common/chip.tsx(1 hunks)src/components/common/modal/SubscriptionModalContent.tsx(1 hunks)src/hooks/api/subscribe/useApiSendSubscribe.ts(1 hunks)src/utils/dateUtil.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/common/Input.tsx
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/app/signup/index.tsx (3)
src/hooks/api/member/useApiSendVerifyEmail.ts (1)
sendVerifyEmail(7-14)src/utils/common.ts (1)
EMAIL_PURPOSE(11-11)src/hooks/api/member/useApiVerifyEmail.ts (1)
verifyEmail(8-16)
src/hooks/api/subscribe/useApiSendSubscribe.ts (1)
src/api/index.ts (1)
api(134-134)
src/components/common/modal/SubscriptionModalContent.tsx (10)
src/hooks/useModal.ts (1)
useModal(9-31)src/hooks/useCheckEmail.ts (1)
useCheckEmail(4-19)src/hooks/api/member/useApiSendVerifyEmail.ts (1)
sendVerifyEmail(7-14)src/utils/common.ts (1)
EMAIL_PURPOSE(11-11)src/hooks/api/member/useApiVerifyEmail.ts (1)
verifyEmail(8-16)src/hooks/api/subscribe/useApiSendSubscribe.ts (1)
sendSubscribeEmail(9-16)src/components/common/chip.tsx (1)
Chip(16-49)src/components/common/Input.tsx (1)
Input(8-63)src/components/app/signup/certificate-box.tsx (1)
CertificateBox(11-49)src/components/common/Button.tsx (1)
Button(9-54)
🔇 Additional comments (16)
src/app/signup/index.tsx (4)
82-82: LGTM! Email verification send button logic is correct.The button is properly disabled when the email is valid but the verification hasn't been sent yet.
84-87: LGTM! API call formatting improved.The
sendVerifyEmailcall is now properly formatted for better readability.
105-105: LGTM! Email verification confirm button logic is correct.The button is properly disabled when the verification code is entered but not yet verified.
110-110: LGTM! Consistent formatting for email purpose parameter.The
emailPurposeparameter formatting is consistent with the previous API call.src/utils/dateUtil.ts (1)
1-7: LGTM! Clean and focused utility implementation.The
formatTimefunction is well-implemented with correct time calculation logic and proper zero-padding for consistent MM:SS format display. This utility effectively supports the countdown timer functionality in the subscription modal.src/api/index.ts (2)
81-82: Good enhancement for extracting detailed error messages.The addition of
descriptionextraction from the API response payload improves error messaging by providing more specific feedback to users when available.
90-113: Consistent application of enhanced error messaging across all status codes.The use of nullish coalescing (
??) ensures proper fallback to default messages while utilizing the extracteddescriptionwhen available. This provides better user experience with more contextual error information.src/hooks/api/subscribe/useApiSendSubscribe.ts (3)
5-5: Good addition of keywords support to subscription interface.The
keywordsproperty properly extends the interface to support the new keyword-based subscription model.
8-8: Helpful TODO comment for future improvements.The TODO comment appropriately identifies the need to refactor this into a proper API hook structure, which would improve consistency with other API interactions.
9-13: Function correctly updated to support keywords parameter.The function signature and API request payload properly include the
keywordsarray, enabling the keyword-based subscription functionality described in the PR objectives.src/components/common/modal/SubscriptionModalContent.tsx (6)
138-138: Clarify keyword validation logic in email verification.The condition
if (keywordList?.[0]?.value?.trim() === "") return;only checks the first keyword. This seems inconsistent with the overall keyword management approach where multiple keywords can be added.Should this check verify that at least one non-empty keyword exists, or is there a specific reason to only check the first keyword?
Consider using:
- if (keywordList?.[0]?.value?.trim() === "") return; + const hasValidKeywords = keywordList.some(k => k.value.trim() !== ""); + if (!hasValidKeywords) return;
166-194: Timer implementation looks good.The timer functionality is well-implemented with proper cleanup, state management, and countdown logic. Good use of cleanup in the effect and proper timer state handling.
211-232: Subscription completion logic is well-implemented.The function properly filters empty keywords, handles the API call asynchronously, and provides appropriate error handling and user feedback.
250-272: Chip component usage is complex but functional.The Chip component integration with both
closableandinputableproperties based on the keyword state works well. The event handlers properly manage the add/edit/remove functionality for keywords.
342-350: Subscription button properly enabled based on conditions.Good improvement from previous reviews - the button is now properly enabled only when email verification is complete and all checkboxes are checked.
356-508: CSS styling is well-organized and follows good practices.The emotion CSS-in-JS styling is properly structured with logical groupings, consistent use of design system variables, and appropriate responsive considerations.
Cllaude99
left a comment
Cllaude99
left a comment
There was a problem hiding this comment.
수고하셨습니다!
disabled와 관련해서 코멘트 남겼습니다. 확인 한 번 부탁드려요~
src/app/signin/index.tsx
Outdated
| margin-top: 5.6rem; | ||
| `} | ||
| state={isValidEmail && password.length > 5} | ||
| disabled={isValidEmail && password.length > 5} |
There was a problem hiding this comment.
로그인 버튼의 비활성화될때에는 !isValidEamil || password.length <= 5 이어야 한다고 생각했어요.
그래서 적어주신 코드에 !를 처리가 필요하다고 생각했어요. !(isValidEmail && password.length > 5) 처럼요!
src/app/signup/consent/index.tsx
Outdated
| margin-top: 1.6rem; | ||
| `} | ||
| state={privacy && useService} | ||
| disabled={privacy && useService} |
There was a problem hiding this comment.
여기도 마찬가지로 '개인정보 수집 및 이용' 이 체크되고 '이용약관'이 체크된 경우에만 다음 버튼이 활성화 되는 것으로 생각했어요!
따라서 !(privacy && useService) 라고 생각이 드는데 어떤지 여쭈어 봅니다!
There was a problem hiding this comment.
b8cd748에서 반영 완료했어요, 감사해요!
충돌 방지하며 뇌를 빼고 코딩했다며.. 🥱
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
목적
작업 내용
필수 리뷰어
이슈 번호
비고
Summary by CodeRabbit
New Features
Enhancements
UI/UX Improvements
Bug Fixes
Chores
Refactor