[MZO-60] 플레이리스트 마크업 작업 #41
Conversation
| images: { | ||
| // NOTE: 임시로 작성 | ||
| remotePatterns: [ | ||
| { | ||
| protocol: 'https', | ||
| hostname: 'via.placeholder.com', | ||
| port: '', | ||
| pathname: '/**', | ||
| }, | ||
| ], | ||
| }, |
There was a problem hiding this comment.
next/image에서 외부 src를 가져오기 위해 임시로 설정해둔 코드입니닷
| @@ -22,7 +22,7 @@ export default function RootLayout({ | |||
| }) { | |||
| return ( | |||
| <html lang="ko" className={`${pretendard.variable}`}> | |||
| <body className="bg-gray-500"> | |||
| <body className="relative m-auto h-screen min-w-[360px] max-w-[480px] bg-gray-500"> | |||
There was a problem hiding this comment.
모든 페이지에서 기본적으로 적용되어야 하는 사항이여서 상위 태그인 body 태그로 옮겼습니다.
| DAY6 | ||
| </p> | ||
| </> | ||
| ) : null} | ||
| </div> | ||
| <div className="flex gap-8 items-center"> | ||
| <div className="flex gap-[23px] items-center"> | ||
| <div className="flex items-center gap-4"> |
There was a problem hiding this comment.
gap, svg크기 등 디자인 시안이 변동된 부분 반영했습니다.
| <AppPortal.Wrapper portalName="modal-portal"> | ||
| <BottomMusicPlayer /> | ||
| </AppPortal.Wrapper> |
There was a problem hiding this comment.
사실 하단 플레이어의 경우 template으로 빼던가 해야할 것 같습니다. 우선 폴더구조가 명확히 잡혀있지 않아 임시로 플레이리스트에 같이 작성해두었습니다.
There was a problem hiding this comment.
각 페이지 마다 계속 해당 컴포넌트를 삽입하기 보다는 template으로 분리하는 게 좋아 보이네요. 가능하다면 그렇게 작업하는 것이 좋아 보입니다. 요구사항이 점점 늘어날수록 변수도 많아지네요.
There was a problem hiding this comment.
맞습니다 아마 나중에 폴더구조가 잡히면 수정하지 않을까 싶어요. 그런데 고민인 부분은, 여기서 Playlist.tsx 페이지가 아닌 컴포넌트로 만들어서 따로 layout을 사용하기가 애매해질것 같긴합니다.. Playing 페이지에서는 하단 뮤직 플레이어바가 안보이고 플레이리스트에서는 보여져야 하는 상황이여서요
| zIndex: { | ||
| modal: 9999, | ||
| toast: 999, | ||
| emojiPicker: 199, | ||
| musicPlayer: 99, | ||
| } |
There was a problem hiding this comment.
사실 다른 포탈이면 z-index는 설정하지 않아도 순서에 따라 원하는대로 배치가 될것 같습니다.
현재는
<AppPortal.Provider portalName="player-portal" />
<AppPortal.Provider portalName="emoji-picker-portal" />
<AppPortal.Provider portalName="modal-portal" />
<AppPortal.Provider portalName="toast-portal">
순으로 되어있는데 마지막에 작성된 포탈이 상위 레이어를 갖게 됩니다.
There was a problem hiding this comment.
그러네요. 마지막으로 작성된 포탈이 더 높은 순위를 가진다라.. 신기하네요. 원인이 뭘까요?
There was a problem hiding this comment.
음 그냥 단순히 html 태그상에서 나중에 작성된 태그가 우선순위가 더 높은거랑 같은 맥락이지 않을까 싶어요!
| @@ -0,0 +1,5 @@ | |||
| import Playlist from './Playlist'; | |||
There was a problem hiding this comment.
개인적으로는 코드의 양이 얼마 되지 않더라도 관심사에 따라 폴더를 분리하는 게 좋다고 생각합니다.
추후 코드의 양이 늘어나게 될 경우 파일 명으로는 각각의 파일이 어떤 역할을 하는지 구분하기 어려워질 것 같아요.
비록 지금은 적은 양이지만 추후 확장성을 고려했을 때는 내부에서 store, hooks를 구분짓는 게 좋다고 생각합니다.
추가적으로 현재 playlist 관련 내용물이 components 폴더에 있는데, 이는 저희가 이전에 이야기 했던 domains 폴더에 넣어서 명확하게 해당 내용들이 어떤 곳에서 쓰이는지를 분리하는 게 좋다고 생각해요. 하랭의 의견은 어떠신가요?
There was a problem hiding this comment.
비록 지금은 적은 양이지만 추후 확장성을 고려했을 때는 내부에서 store, hooks를 구분짓는 게 좋다고 생각합니다.
좋습니다!! 수정하겠습니다~~
추가적으로 현재 playlist 관련 내용물이 components 폴더에 있는데, 이는 저희가 이전에 이야기 했던 domains 폴더에 넣어서 명확하게 해당 내용들이 어떤 곳에서 쓰이는지를 분리하는 게 좋다고 생각해요. 하랭의 의견은 어떠신가요?
아 요 부분은 Playlist의 경우 공통 컴포넌트라고 생각해서 components 폴더에 두긴 했는데.. 약간 애매한 부분이 있는것 같긴 하네요. domains폴더와 components 폴더를 확실히 구분해야할것 같아요. 저는 딱 한번쓰이는건 domain에, 두번 이상 쓰인다면 componet에 두는걸로 인식했었는데 어떻게 하는게 좋을까요??
|
|
||
| const useToastAtom = atom( | ||
| (get) => get(playlistAtom).isOpenPlaylist, | ||
| (get, set, type: string) => { |
There was a problem hiding this comment.
개인 취향이긴 합니다만 현재는 플레이 리스트가 "열렸는지 아닌지" 만을 체크하는 상황이기 때문에 굳이 switch 문이 필요하지 않다고 생각해요.
setState 자체를 넘기는 게 아니라 usePlaylist 훅에서 플레이리스트 열림 여부를 제어하는 함수 2개를 반환하기 때문에 괜찮다고 생각합니다.
const useToastAtom = atom(
(get) => get(playlistAtom).isOpenPlaylist,
(get, set, isOpenPlaylist: boolean) => {
set(playlistAtom, {
isOpenPlaylist,
});
}
},
);
// 중략...
return {
isOpenPlaylist,
openPlaylist: () => setIsOpenPlaylist(true),
closePlaylist: () => setIsOpenPlaylist(false),
};There was a problem hiding this comment.
아 좋습니다! 사실 switch문을 사용한 이유는 저기서 추가로 togglePlaylist를 만들었어서 그랬는데 토글은 딱히 필요가 없을것 같아서 빼뒀었습니다.
루키가 제안주신 코드로 수정해둘게요!
| @@ -43,17 +43,53 @@ module.exports = { | |||
| 800: '#444444', | |||
| 900: '#262626', | |||
| }, | |||
| blue: { | |||
| <AppPortal.Wrapper portalName="modal-portal"> | ||
| <BottomMusicPlayer /> | ||
| </AppPortal.Wrapper> |
There was a problem hiding this comment.
각 페이지 마다 계속 해당 컴포넌트를 삽입하기 보다는 template으로 분리하는 게 좋아 보이네요. 가능하다면 그렇게 작업하는 것이 좋아 보입니다. 요구사항이 점점 늘어날수록 변수도 많아지네요.
| @@ -29,6 +29,7 @@ const PortalProvider = ({ | |||
| return ( | |||
| <PortalContext.Provider value={portalList}> | |||
| <div | |||
| className="absolute top-0 left-0 w-full" | |||
There was a problem hiding this comment.
div 생성 시 바로 스타일링 입혀주는 것도 제가 놓친 부분인데 채워주셔서 감사합니다.
src/components/playlist/Playlist.tsx
Outdated
| import Image from 'next/image'; | ||
| import React from 'react'; | ||
|
|
||
| import Close from '@/assets/icons/close.svg'; |
There was a problem hiding this comment.
[NIT]
저는 아이콘 관련 SVG를 import 할 때 접미사로 Icon 을 붙이는 편인데 이것도 통일하는 게 좋을까요?
|
|
||
| interface PlaylistButtonProps | ||
| extends React.ButtonHTMLAttributes<HTMLButtonElement> { | ||
| className?: React.ComponentProps<'button'>['className']; |
There was a problem hiding this comment.
타입 진짜 야무지네요. 제가 아직 이런 식의 타입 사용이 부족한데 많이 배웁니다.
Jira Issue 번호
#60
작업 내용
To Reviewer
Checklist