[feat] 회원가입 로직 수정 #11
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the signup logic by improving field naming, adding explicit agreement fields for terms and privacy, and simplifying the routing structure. The changes replace the generic "school" concept with "university" throughout the codebase for clarity, and consolidate the signup flow by removing nested routes.
Key Changes
- Renamed signup state fields to be more explicit:
marketing→isMarketingAgreement,school→universityId, and addedisTermsAgreementandisPrivacyAgreementfields - Refactored
SchoolStepintoUniversityStepwith improved search functionality and removed the custom input option - Flattened signup route structure from
/signup/profile/schoolto/signup/universityfor simpler navigation
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/stores/signupStore.ts | Updated state interface with explicit agreement fields and renamed school to universityId |
| src/pages/SignUp/UniversityStep.tsx | New component replacing SchoolStep with search-based university selection |
| src/pages/SignUp/TermStep.tsx | Updated to persist all three agreement states to store and navigate to flattened route |
| src/pages/SignUp/StudentIdStep.tsx | Updated to use flattened navigation path |
| src/pages/SignUp/SchoolStep.tsx | Removed file replaced by UniversityStep |
| src/pages/SignUp/NameStep.tsx | Updated to use universityId and isMarketingAgreement in API call |
| src/apis/auth/entity.ts | Updated SignupRequest interface to match new field names |
| src/App.tsx | Simplified routing structure by removing nested profile routes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/pages/SignUp/UniversityStep.tsx
Outdated
| import StepLayout from './components/StepLayout'; | ||
| import { useGetUniversityList } from './hooks/useUniversity'; | ||
|
|
||
| function SchoolCard({ label, onClick }: { label: string; onClick: () => void }) { |
There was a problem hiding this comment.
The component name 'SchoolCard' is inconsistent with the file and feature naming. Since this file is now 'UniversityStep' and deals with universities, the component should be renamed to 'UniversityCard' for consistency.
| const handleNext = () => { | ||
| mutate( | ||
| { | ||
| name, | ||
| school, | ||
| universityId, | ||
| studentNumber: studentId, | ||
| isMarketingAgreement, | ||
| }, |
There was a problem hiding this comment.
The name field is not being persisted to the store before submitting. Unlike the previous steps (TermStep and StudentIdStep) which call update() to save their values to the store, this step goes directly to the API call. If the API call fails and the user navigates back and forth, the entered name will be lost. Consider calling update({ name }) before the mutate() call to maintain consistency with other steps.
| <input | ||
| type="text" | ||
| value={universityName} | ||
| placeholder="학교를 입력해주세요.." |
There was a problem hiding this comment.
The placeholder text has an inconsistent number of periods. It ends with two periods '..' but should end with a single period '.' for consistency with standard Korean punctuation.
| placeholder="학교를 입력해주세요.." | |
| placeholder="학교를 입력해주세요." |
| const trimmed = universityName.trim(); | ||
| const filteredUniversities = !trimmed | ||
| ? (universityList?.universities ?? []) | ||
| : (universityList?.universities.filter((university) => university.name.includes(trimmed)) ?? []); |
There was a problem hiding this comment.
The filtering logic recalculates on every render. If the university list is large, this could cause performance issues. Consider using useMemo to memoize the filtered results, especially since the calculation depends only on universityName and universityList.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
In this project, we are using the React Compiler, so this kind of calculation is automatically memoized by the compiler.
Since the filtering logic is a pure computation that depends only on universityName and universityList, React Compiler can safely optimize it without needing an explicit useMemo.
| <input | ||
| type="text" | ||
| value={universityName} | ||
| placeholder="학교를 입력해주세요.." | ||
| onChange={(e) => setUniversityName(e.target.value)} | ||
| className="mt-5 w-full border-b-2 border-indigo-400 py-4 text-[20px] font-bold text-indigo-300 placeholder:text-indigo-300" |
There was a problem hiding this comment.
The search input lacks proper accessibility attributes. Consider adding an aria-label or id with associated label for screen readers, and aria-describedby to connect it with the description text. This helps users with assistive technologies understand the input's purpose.
| <input | |
| type="text" | |
| value={universityName} | |
| placeholder="학교를 입력해주세요.." | |
| onChange={(e) => setUniversityName(e.target.value)} | |
| className="mt-5 w-full border-b-2 border-indigo-400 py-4 text-[20px] font-bold text-indigo-300 placeholder:text-indigo-300" | |
| <input | |
| id="university-search" | |
| type="text" | |
| value={universityName} | |
| placeholder="학교를 입력해주세요.." | |
| onChange={(e) => setUniversityName(e.target.value)} | |
| className="mt-5 w-full border-b-2 border-indigo-400 py-4 text-[20px] font-bold text-indigo-300 placeholder:text-indigo-300" | |
| aria-label="학교 검색" |
src/pages/SignUp/UniversityStep.tsx
Outdated
| ? (universityList?.universities ?? []) | ||
| : (universityList?.universities.filter((university) => university.name.includes(trimmed)) ?? []); | ||
|
|
||
| const handleSchoolSelect = (universityId: string) => () => { |
There was a problem hiding this comment.
The handler function is named 'handleSchoolSelect' but should be renamed to 'handleUniversitySelect' for consistency with the component and feature naming convention.
No description provided.