-
Notifications
You must be signed in to change notification settings - Fork 0
Feat/file upload #19
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
Feat/file upload #19
Conversation
|
|
💄 Storybook: https://67c9a019e2c059ab48fd9565-yiystyjmgj.chromatic.com/ |
jongjunpark
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.
일단 표면적인것만 작성했고 이따가 저녁에 세부하게 다시볼게
| import type { Metadata } from "next"; | ||
| import { Geist, Geist_Mono } from "next/font/google"; | ||
| import "./globals.css"; | ||
| // import "@repo/ui/src/globalStyle/global.css.ts"; |
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.
추가할건데 alias 안되서 우선 주석처리 해놨음!
apps/admin/tsconfig.json
Outdated
| "paths": { | ||
| "@/*": ["./src/*"] | ||
| "@/*": ["./src/*"], | ||
| "@repo/ui/*": ["../../packages/ui/*"] |
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.
얘도 지우고 install해서 사용하는걸로
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.
컴파일돼서 생긴건가?
| config.plugins = config.plugins || []; | ||
| config.plugins.push( | ||
| vanillaExtractPlugin({ | ||
| identifiers: ({ hash }) => `_${hash}`, |
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.
indetifiers 요거 저거 어디에 쓰는지 궁금해서 저거없어도 돌아가긴 하거든
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.
https://vanilla-extract.style/documentation/integrations/vite/#identifiers
음 지정안해도 기본값 설정이 있다고는 하네
apps/storybook/package.json
Outdated
| "dev": "storybook dev -p 6006", | ||
| "build-storybook": "storybook build" | ||
| "build-storybook": "storybook build", | ||
| "storybook": "start-storybook -p 6006" |
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.
dev가 이거랑 동일한 역할을 하고 있긴함. 그리고 굳이 apps/storybook까지 와서 pnpm storybook을 쓸 필요없이 루트에 "storybook": "pnpm --filter storybook dev"을 추가해뒀기 때문에 루트에서 pnpm storybook하면 실행됨
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.
그럼 8번 줄만 삭제하면 될까욥
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.
예얍 삭제하고 apps/storybook 경로에서는 pnpm dev로 하고, root에서는 pnpm storybook으로 쓰면됨!
apps/user/tsconfig.json
Outdated
| "paths": { | ||
| "@/*": ["./src/*"] | ||
| "@/*": ["./src/*"], | ||
| "@repo/ui/*": ["../../packages/ui/*"] |
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.
PascalCase랑 snake_case를 혼용해서 사용해 본적이 없는데
하나만 쓰면 좋을 것 같음
| }, | ||
| }); | ||
|
|
||
| export const Color = createGlobalTheme(":root", { |
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.
요기 몇몇 색깔은 ColorVar로 대체하면 좋을듯?
| @@ -0,0 +1,21 @@ | |||
| import { create } from "zustand"; | |||
| import { UrlType } from "../types/imageUrlType"; | |||
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.
type은 import type으로 됐으면 좋겠음.
lint에 https://typescript-eslint.io/rules/consistent-type-imports/ 요거 추가하는걸로
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.
폴더명은 camel인데, 파일명은 Pascal인게 어색해서,
camel/camel로 하던지 Pascal/Pascal로 해서 통일했으면 함
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.
그럼 컴포넌트가 들어가있는 폴더만 파스칼로 바꾸고 나머진 camel 유지하면 될까요
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.
응응 그렇게 통일하자 그러면
jongjunpark
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.
양이 많네... ㅋㅋ 기능은 잘동작하는거 확인함 잘만들었네
몇개 생각한거 쭉 적어둘게 TODO list라고 생각해
- 고용량 처리 어떻게 할지?
- dnd가 과연 폰에서도 잘 될지?
- 아키텍처 구분이 필요
- globalStyle, store랑 같은 level이면 안됨
- 동일한 컴포넌트는 하나의 폴더내에서 구분필요
- 폴더마다 style.css.ts 두는게 나을지 논의 필요
- px 표시하는걸로
- Immer 알아보기
- spell checker
| const newUrlList = [...urlList]; | ||
| for (let file of files) { | ||
| const url = URL.createObjectURL(file); | ||
| const uuid = crypto.randomUUID(); | ||
| const urlObj = { | ||
| id: uuid, | ||
| url, | ||
| }; | ||
| newUrlList.push(urlObj); | ||
| setUrlList(newUrlList); | ||
| } |
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 newUrlList = [...urlList]; | |
| for (let file of files) { | |
| const url = URL.createObjectURL(file); | |
| const uuid = crypto.randomUUID(); | |
| const urlObj = { | |
| id: uuid, | |
| url, | |
| }; | |
| newUrlList.push(urlObj); | |
| setUrlList(newUrlList); | |
| } | |
| setUrlList([ | |
| ...urlList, | |
| ...Array.from(files).map((file) => ({ | |
| id: crypto.randomUUID(), | |
| url: URL.createObjectURL(file), | |
| })), | |
| ]); |
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.
깊은 복제 후에 push 하기 보다는 더 간편하게 할 수 있음.
그리고 for문을 쓰기보다는 map, forEach 같이 좀 더 선언적인 방법을 사용하는 것을 추천
| const handleSelectFile = (e: ChangeEvent<HTMLInputElement>) => { | ||
| const files = e.target.files || []; | ||
| uploadFiles(files as FileList); | ||
| }; |
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 handleSelectFile = (e: ChangeEvent<HTMLInputElement>) => { | |
| const files = e.target.files || []; | |
| uploadFiles(files as FileList); | |
| }; | |
| const handleSelectFile = (e: ChangeEvent<HTMLInputElement>) => { | |
| const files = e.target.files; | |
| if (!files) return; | |
| uploadFiles(files); | |
| }; |
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.
억지로 as를 쓰는 방법보다는 early return을 사용해서, 값이 없는 경우에는 해당 루트를 타지않도록 설정 추천
| const handleDragOver = (e: React.DragEvent) => { | ||
| e.preventDefault(); | ||
| }; |
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.
이건 안쓰는거 같은데 빼는게 나을듯?
| return ( | ||
| <InputWrapper> | ||
| <label | ||
| className={isActive ? `${style.label.active}` : `${style.label.none}`} |
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.
삼항연산자로 class명을 바꾸기 보다는 recipe를 써보는건 어때?
| name="image" | ||
| type="file" | ||
| multiple | ||
| accept="image/png, image/jpeg, image/jpg" |
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.
ngrok 스토리북 컴포넌트 로딩이 안돼서 이따 같이 함 봐주시오
| } | ||
| } | ||
| }; | ||
| const hadleDragEnter = (e: React.DragEvent) => { |
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.
어떤ㄱ ㅓ,, ?
| return ( | ||
| <div className={style.wrapper}> | ||
| <ImageInput /> | ||
| <ul className={style.previewList}> |
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.
오 ul + li 쓴거 상당히 칭찬해
| export const previewList = style({ | ||
| display: "flex", | ||
| gap: 8, | ||
| justifyContent: "start", |
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.
start와 flex-start의 차이를 한번 알아보기
| position: "absolute", | ||
| top: 0, | ||
| right: 0, | ||
| transform: "translateY(-30%) translateX(30%)", |
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.
transform으로 처리한 이유가 궁금합니다
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.
버튼 위치 조정하려고 했는데 그냥 top, right에 값을 주는게 나을까요
| "compilerOptions": { | ||
| "outDir": "dist" | ||
| "outDir": "dist", | ||
| "typeRoots": ["./node_modules/@types", "./src/types"] |
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.
"./node_modules/@types" 요거 필요한가요?
jongjunpark
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.
👍
색상 변수는 추후 수정 및 스토리북 팔레트 추가 예정