-
Notifications
You must be signed in to change notification settings - Fork 8
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
템플릿 생성 및 수정페이지 컴포넌트 및 훅 분리 #694
Conversation
…wCategoryInput 컴포넌트 분리
…@/hooks/template 으로 변경, useSourceCodeSelectList 내부에서 index로 currentFile 감지하도록 통일
… 을 감싸는 스타일드 컴포넌트 사용하도록 변경
<S.FilenameInput | ||
value={filename} | ||
onChange={handleFilenameChange} | ||
placeholder={'파일명.[확장자]'} |
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.
기존 placeholder 파일명.js
에서 js
가 눈에 잘 띄지 않는다는 피드백과 js
가 프론트엔드에게만 친숙하다는 점을 고려하여 파일명.[확장자]
로 변경해보았습니다!
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.
- 처음에는 함께 논의했던 것처럼 useInput 에 두번째 인자로 isAllowedSpace 라는 boolean인자를 받으려고 했으나 useNoSpaceInput 가 더 명시적이고 선언적이라고 생각하여 새로운 훅을 생성하였습니다.
// useInput 확장한 경우 1
// 두번째 인자가 무엇을 의미하는지 useInput에 들어가야만 파악할 수 있음
const [value, setValue] = useInput('', false);
// useInput 확장한 경우 2
// 불필요하게 선언부가 길어지는 느낌이 듬
const [value, setValue] = useInput({initValue: '', isAllowedSpace: false});
// useNoSpaceInput 생성한 경우
// 훨씬 선언적이고 명시적으로 느껴짐
const [value, setValue] = useNoSpaceInput('');
frontend/src/hooks/useToast.ts
Outdated
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.
useAuth
와 같이, 명시적으로 훅을 하나만 불러서 사용할 수 있도록 useToast
를 생성하였습니다.
// 변경 전
// import 도 2개 해왔어야 함
const { infoAlert } = useCustomContest(ToastContext);
// 변경 후
// 훨씬 편리하고, import 도 1개만 해오면 됨
const { infoAlert } = useToast();
frontend/src/service/validates.ts
Outdated
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.
해당 파일에서는 fileName => filename 만 변경되었습니다.
frontend/src/service/index.ts
Outdated
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.
해당 파일에서는 fileName => filename 만 변경되었습니다.
@@ -35,7 +35,7 @@ export const uploadTemplateToCodezap = async ({ | |||
} | |||
|
|||
// 파일명 입력 | |||
await page.getByPlaceholder('파일명.js').fill(fileName); | |||
await page.getByPlaceholder('파일명.[확장자]').fill(fileName); |
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.
- 하단 SourceCodeEditor 파일 코멘트 참조 부탁드립니다.
코멘트 내용: 기존 placeholder 파일명.js 에서 js 가 눈에 잘 띄지 않는다는 피드백과 js 가 프론트엔드에게만 친숙하다는 점을 고려하여 파일명.[확장자] 로 변경해보았습니다!
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.
흠 테스트용인데도 그러려나 싶었는데, 뭐로 해도 무방할 것 같긴 하네요@!
0774ee2
to
c43a5b0
Compare
현재 dev/fe 를 받아 Conflict 해결할 수 있으나, 해결 후 파일 변경 사항이 너무 많이 잡히는 것을 확인하여 일단 코드리뷰 후 해결한 커밋을 올리도록 하겠습니다. |
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.
너무 고생했습니다! 이전보다 분리도 잘 되어있고 가독성도 좋네요.
얼마나 고민했는지 보이고 시간도 꽤 지나서 Approve할까 하다가 제가 남긴 코멘트를 고치고 싶어할 수도 있을거 같아서 우선 남겨둡니다
@@ -35,7 +35,7 @@ export const uploadTemplateToCodezap = async ({ | |||
} | |||
|
|||
// 파일명 입력 | |||
await page.getByPlaceholder('파일명.js').fill(fileName); | |||
await page.getByPlaceholder('파일명.[확장자]').fill(fileName); |
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.
흠 테스트용인데도 그러려나 싶었는데, 뭐로 해도 무방할 것 같긴 하네요@!
@@ -35,7 +35,7 @@ test('템플릿 제목, 설명, 파일명, 소스코드, 태그를 입력하고 | |||
tag: testTitle, | |||
}); | |||
|
|||
const templateCard = page.getByRole('link', { name: `testTitle` }).first(); | |||
const templateCard = page.getByRole('link', { name: testTitle }).first(); |
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.
👍
@@ -50,7 +50,7 @@ test('템플릿 카드를 누르면 템플릿 제목, 설명, 작성자, 생성 | |||
}) => { | |||
await page.goto('/my-templates'); | |||
// 템플릿 목록 | |||
await waitForSuccess({ page, apiUrl: '/templates' }); | |||
await waitForSuccess({ page, apiUrl: '/templates/login?keyword' }); |
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.
엔드포인트가 얼른 고쳐졌으면 좋겠군요..! ㅜㅜ
<Guide isOpen={isOpen} css={{ marginTop: '0.5rem', marginBottom: '-0.5rem' }}> | ||
{isError ? ( | ||
<Text.Small color={theme.color.light.analogous_primary_300}>{categoryErrorMessage}</Text.Small> | ||
) : ( | ||
<Text.Small color={theme.color.light.secondary_400}>엔터로 카테고리를 등록해요</Text.Small> | ||
)} | ||
</Guide> |
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.
이번 PR에서의 변경 사항은 아니지만 이야기해보고 싶어서 남겨둡니다!
해당 부분에서 Guide에 css를 추가로 주는게 어색한거 같아서 생각하게 된건데,
현재는 CategoryGuide와 Dropdown이 같은 계층에 있어서 Guide라는 공통 컴포넌트를 두고 이런 css 속성을 쓰게 되었던 것 같네요. CategoryDropdown이라는 container 개념의 컴포넌트에 Guide와 Dropdown이 있어도 좋을거 같다는 생각입니다!
DropdownInput 컴포넌트로 만들고 Dropdown+Input+Guide 형식의 합성 컴포넌트처럼 만들어 봐도..? 다른 곳에서도 비슷한 기능을 쓸 수도 있을 것 같아서요. 익스텐션에선 씁니다.. ㅜ
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.
Dropdown+Input+Guide 형식의 합성 컴포넌트도 좋은 아이디어네요!
그런데 하루안에 구현할 자신은 없기때문에 다음 리팩토링에서 고려해보겠습니다!!
handleContentChange?: (value: string, viewUpdate: ViewUpdate) => void; | ||
} | ||
|
||
const SourceCode = ({ mode = 'detailView', language, content, handleContentChange }: Props) => { |
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.
👍👍
<ChevronIcon /> | ||
</S.SourceCodeToggleIcon> | ||
<S.NoScrollbarContainer> | ||
<Text.Small color='#fff' weight='bold'> |
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.
혹시 여기도 theme에서 써야할까요? ㅎㅎ white이긴 한데
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.
써야죠!! theme 으로 변경하였습니다~!
<S.SourceCodeWrapper isOpen={isSourceCodeOpen}> | ||
{isSourceCodeOpen && <SourceCode mode={mode} content={content} language={getLanguageByFilename(filename)} />} | ||
</S.SourceCodeWrapper> |
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.
이때가 mode가 detailView가 아닌 Thumbnail일 때 인가요?
위에서 mode === 'detailView' 일때는 명확히 보이는데, 만약 mode?: Exclude<SourceCodeMode, 'edit'>;
로 타입에서 edit을 제외해서 무조건 해당 부분은 있으니 그런거겠죠?!
아하 thumbnail일때는 content를 5줄 밖에 안주니 detailView일때만 위의 filename 부분이 필요하군요.
이렇게 보니 굳이 mode를 3개로 나누지 않고도, filename이 있냐 없냐에 따라 detailView와 Thumbnail이 구분될거 같고, edit이 가능한지만 구분해도 될거 같다는 생각이 드네요
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.
filename이 있냐 없냐에 따라 결정해볼까 생각도 했으나, 추후 Thumbnail 에서도 파일 이름을 보여주기로 바뀔수도 있고, detailView와 Thumbnail로 명시적으로 타입을 나누는 것이 보기에 더 이해가 쉬울 것이라고 생각하여 mode 를 3가지로 구분해보았습니다!
또한 SourceCode
에서 아래와 같이 스타일을 다르게 주어야 하기 때문에 필요하다고 생각하였습니다!
39 height={mode === 'thumbnailView' ? '10rem' : '100%'}
if (!sourceCodes[index]) { | ||
console.error('존재하지 않는 소스코드는 삭제할 수 없습니다. 삭제하려는 소스코드의 index를 다시 확인해주세요.'); | ||
} |
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.
이 부분을 console.error로 처리하고 있었는데, 사용자한테는 어떻게 나타내면 좋을지 고민해봐야겠네요..
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.
사실 이 부분은 사용자에게 보이면 안되는 부분이긴 합니다...! 이 부분은 개발자를 위한 에러라고 생각했는데요, 이렇게 사용자를 위한 에러와 동료 개발자를 위한 에러를 구분해서 잘 나타내는 방법에 대해서도 함께 고민해보면 좋을 것 같네요!
export const useNoSpaceInput = (initValue: string = '') => { | ||
const [value, setValue] = useState(initValue); | ||
|
||
const handleChange = (e: React.ChangeEvent<HTMLInputElement>) => setValue(e.target.value.trim()); | ||
|
||
const resetValue = () => { | ||
setValue(''); | ||
}; | ||
|
||
return [value, handleChange, resetValue] as const; | ||
}; |
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.
useTrimInput
같은 네이밍은 어떨지 작은 제안 드려봅니닷
noSpace라고 하니까 아예 띄어쓰기를 못할 것 같은데, 중간에 띄어쓰기가 있는 문자열을 복붙하면 될 것 같아서요
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.
사실 아예 띄어쓰기를 못하게 하는 것이 목표였는데, 중간에 띄어쓰기가 있는 문자열을 복붙하는 경우를 고려하지 못했네요!
현재 useNoSpaceInput
가 쓰이는 곳은 TagInput
인데요, 태그는 띄어쓰기를 허용하고 있지 않습니다.
만약 중간에 띄어쓰기가 있는 문자열을 복붙하는 경우에도 input에서는 띄어쓰기가 보이지만, 태그를 등록하면 띄어쓰기 없이 등록이 됩니다! 따라서 해당 input 에서도 띄어쓰기가 아예 보이지 않도록 수정하겠습니다!
export const useSelectList = (sourceCodes: SourceCodes[]) => { | ||
export const useSelectList = () => { | ||
const scrollTo = useScrollToTargetElement(); |
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.
👍
if (!context) { | ||
throw new Error('useAuth must be used within an AuthProvider'); | ||
} |
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.
(질문) AuthProvider
미사용 경고를 삭제한 이유가 궁금합니당
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.
useCustomContext
을 사용해서 구현하도록 변경하였기 때문입니다!
useCustomContext
내부에서 아래와같이 context가 유효한지 검증합니다!
if (!value) {
throw new Error('컨텍스트가 존재하지 않습니다.');
}
<div | ||
id={sourceCode.filename} | ||
<SourceCodeViewer | ||
key={sourceCode.id} | ||
ref={(el) => (sourceCodeRefs.current[index] = el)} | ||
css={{ width: '100%' }} | ||
> | ||
<Flex | ||
justify='space-between' | ||
align='center' | ||
height='3rem' | ||
padding='1rem 1.5rem' | ||
width='100%' | ||
style={{ background: `${theme.color.light.tertiary_600}`, borderRadius: '8px 8px 0 0' }} | ||
> | ||
<Flex | ||
justify='space-between' | ||
align='center' | ||
gap='0.5rem' | ||
onClick={handleIsOpenList(index)} | ||
css={{ | ||
cursor: 'pointer', | ||
flex: 1, | ||
minWidth: 0, | ||
}} | ||
> | ||
<ChevronIcon | ||
width={16} | ||
height={16} | ||
aria-label='소스코드 펼침' | ||
css={{ | ||
transition: 'transform 0.3s ease', | ||
transform: isOpenList[index] ? 'rotate(180deg)' : 'rotate(0deg)', | ||
}} | ||
/> | ||
<S.NoScrollbarContainer> | ||
<Text.Small color='#fff' weight='bold'> | ||
{sourceCode.filename} | ||
</Text.Small> | ||
</S.NoScrollbarContainer> | ||
</Flex> | ||
<Button | ||
size='small' | ||
variant='text' | ||
onClick={copyCode(sourceCode)} | ||
css={{ | ||
width: 'auto', | ||
padding: '0.25rem 0.5rem', | ||
whiteSpace: 'nowrap', | ||
minWidth: 'fit-content', | ||
}} | ||
> | ||
<Text.Small color={theme.color.light.primary_500} weight='bold'> | ||
{'복사'} | ||
</Text.Small> | ||
</Button> | ||
</Flex> | ||
<S.SyntaxHighlighterWrapper isOpen={isOpenList[index]}> | ||
{isOpenList[index] && ( | ||
<CodeMirror | ||
value={sourceCode.content} | ||
height='100%' | ||
style={{ width: '100%', fontSize: '1rem' }} | ||
theme={quietlight} | ||
extensions={[ | ||
loadLanguage(getLanguageByFilename(sourceCode?.filename) as LanguageName) || [], | ||
S.CustomCodeMirrorTheme, | ||
EditorView.editable.of(false), | ||
]} | ||
css={{ | ||
'.cm-editor': { | ||
borderRadius: '0 0 8px 8px', | ||
overflow: 'hidden', | ||
}, | ||
'.cm-scroller': { | ||
padding: '1rem 0', | ||
overflowY: 'auto', | ||
height: '100%', | ||
}, | ||
}} | ||
/> | ||
)} | ||
</S.SyntaxHighlighterWrapper> | ||
</div> | ||
mode='detailView' | ||
filename={sourceCode.filename} | ||
content={sourceCode.content} | ||
sourceCodeRef={(el) => (sourceCodeRefs.current[index] = el)} | ||
/> |
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.
굿굿👍🏻👍🏻⭐
import { validateFilename, validateSourceCode } from '@/service'; | ||
import { SourceCodes } from '@/types'; | ||
|
||
import { useToast } from '../useToast'; |
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.
(질문) 공용 유틸 훅들은 common
으로 폴더링 하면 어떨까요?
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.
공용 유틸은 그냥 외부 utils 에 두기로 했었던 것 같아서..! 일단 별도의 논의가 따로 진행되기 전까진 폴더링은 유지하도록 하겠습니다!
[failAlert], | ||
); | ||
|
||
const handleContentChange = useCallback((newContent: string, idx: number) => { |
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.
useCallback
의 사용이 좋다고 생각됩니다! 👍🏻
useCallback
를 사용하지 않고 있는 다른 파일에서도 적용할 것인지 궁금합니당
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.
커스텀 훅에서 반환하는 함수들은 useCallback
을 사용하는 편이 좋다고 알고있어서 저는 그렇게 구현하는 편인데, 다른 분들의 생각도 동일하면 일관성있게 커스텀훅에서 내보내는 모든 함수를 감싸주는 것도 좋을 것 같네요!! 월하의 생각은 어떤가요?
…de-zap into refactor/649-template-edit
const [value, setValue] = useState(initValue); | ||
|
||
const handleChange = (e: React.ChangeEvent<HTMLInputElement>) => { | ||
const noSpaceValue = e.target.value.replace(/[\s\u3164\uFEFF\u200B]+/gu, ''); |
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.
모든 공백을 막기 위함입니다.
ex) ▷ㅤㅤ◁ 공백처럼 보이게 하는 유니코드 (u3164)
\u3164: 한글 자음 필러 (ㅤ)와 일치
\uFEFF: 제로 폭 공백(Zero Width No-Break Space)과 일치
\u200B: 제로 폭 공백(Zero Width Space)과 일치
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.
이야 유니코드.... 고생했습니다
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.
다음과 같은 여러가지 공백 문자들에 대해서도 동일하게 처리해주어야 하지 않을까해서 남겨봐용
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.
안녕하세요, 프론트 여러분! 헤인입니다.
먼저 변경사항이 무척 많이 잡히고 있는데, 그 이유는 컴포넌트를 분리하면서 생성/삭제/이름이 변경된 파일들이 많기 때문입니다.
(물론 리팩터링 과정에서 변경사항이 많은 것도 사실입니다🤣)
보다 원할한 코드리뷰를 하실 수 있도록 최대한 부연설명과 구현할 때 고민했던 지점에 대해 적어두었습니다.
이외에 궁금하신 점이 더 있으시다면 코멘트 달아주세요!!
⚡️ 관련 이슈
🪧 이번 리팩터링의 주요 과제
📍주요 변경 사항
1.
TemplateEdit
컴포넌트 및useTemplateUpload
,useTemplateEdit
hook 삭제이번 리팩터링의 주요 과제
참고 부탁드립니다.2. 서브컴포넌트 별도의 파일로 분리
서브컴포넌트를 페이지 컴포넌트 내부에 두었을 때, 하단에 분리되어있다는 것을 한 눈에 인지하기 어렵다는 문제 제기됨
=> 파일을 분리하여 명시적으로 import 해오도록 수정하자고 팀 내부에서 합의했기 때문에 아래와 같이 분리!
TemplateEdit 에서
CategoryDropdown
,CategoryGuide
,NewCategoryInput
별도의 컴포넌트로 파일 분리TemplateUploadPage
와TemplateEditPage
에서 함께 사용하기 때문에@src/components
에 위치CategoryDropdown
,CategoryGuide
,NewCategoryInput
의 관계3.
SourceCode
,SourceCodeEditor
,SourceCodeViewer
컴포넌트 분리4. 커스텀 훅 세분화 및 역할 강화
useTag
,useSourceCode
생성useCategory
훅에서 카테고리 생성에 대한 함수도 반환하도록 변경useToast
생성 (하단 코멘트로 생성 이유 달아두었습니다.)useNoSpaceInput
생성 (하단 코멘트로 생성 이유 달아두었습니다.)5. selectList 의 현재 선택된 파일 상태(
currentFile
)를index
로 통일변경 전 : 템플릿 상세페이지에서는
id
, 템플릿 생성 및 수정페이지에서는index
로 현재 파일 상태를 설정하고 있었음=> 이로인해 불필요한 함수 생김
=>
id
가 더 명시적이긴 하지만, 복잡한 로직을 감수해야 할 만큼 중요한 정보 아니라고 판단하여 변경하게 됨변경 후 : 모든 페이지에서
index
로 현재 파일 상태를 저장하도록 통일6.
filename
으로 용어 통일filename
과fileName
을 혼재하여 사용하고 있는 것을 발견=>
cSpell
에서 한 단어로 보기도 하고, 백엔드에서도filename
으로 사용하고 있기 때문에filename
으로 통일🎸기타
[우려 사항]
[개인적인 고민]
=> 그러나 지금 depth 및 복잡성을 줄이기 위해 결국 중복을 허용하게 되었습니다.
=> 개인적으로 결국 예전 설계로 회귀한 기분이 듭니다. 문제를 완전히 해결하기보다는 이전 문제로 되돌아온 기분이랄까요?
=> 과연 이 문제가 트레이드오프라 반드시 하나를 택해야 하는 문제인지 아니면 더 좋은 돌파구가 있을지 다른 분들의 견해가 궁금합니다!