-
Notifications
You must be signed in to change notification settings - Fork 2
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 : 북마크 문장 저장하는 사이드 바 구현, 버튼과 로직 연결 #40
Conversation
bookMarkedSentences: string[]; | ||
toggleBookmarking: (sentence: string) => void; | ||
} | ||
export default function ResponsiveSidebar({ |
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.
뭔가 이름에 responsive가 들어가지 않아도 될 것 같아요! bookmarksidebar 같은 방식이 좀 더 어떤 컴포넌트인지 확실히 알 수 있을 것 같습니다
}: ResponsiveSidebarProps) { | ||
const [isSidebarOpen, setIsSidebarOpen] = useState(false); | ||
const toggleSidebar = () => { | ||
setIsSidebarOpen(!isSidebarOpen); |
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.
이전 값을 좀 더 명확히 참조하도록 prev=> !prev 이런식으로 작성하는게 어떨까요?
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.
setIsSidebarOpen훅 이름만 보고 sidebar의 open상태를 조절하는 기능을 한다는 걸 알 수있지만, 저는 보통 조절하는 상태의 값을 적어주는 게 더 직관적이라고 생각해서 원래의 isSidebarOpen이라는 구체적인 상태값을 적어주는 편이기는 합니다. prev로 적으면 이전값을 변경한다는 의미가 박히지만 어떤 값을 바꾸는지 직관적으로 안 다가와서요..!
{/* todo : drag & drop으로 위치 변경 가능하게 만들기? or 배경화면 투명하게 */} | ||
{/* todo : 모바일 화면이면 30%민 보이도록 */} | ||
{/* 사이드바 콘텐츠 */} | ||
<ScrollArea className="h-[100%] w-[100%] p-4 scrollbar"> |
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.
shadcn에 scrollarea도 있군요!
${isSidebarOpen ? 'translate-x-0' : 'translate-x-full'} | ||
xl:w-[300px] `} | ||
> | ||
{/* todo : drag & drop으로 위치 변경 가능하게 만들기? or 배경화면 투명하게 */} |
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.
넵 그러면 이 주석은 투두로 남겨두겠습니다
// 북마크된 문장을 관리하는 배열 | ||
const [bookMarkedSentences, setBookMarkedSentences] = useState<string[]>([]); | ||
|
||
const toggleBookmarking = (sentence: string) => { |
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.
이름은 약간 toggleBookmark가 더 익숙한 느낌일 것 같습니다.
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.
동명사Bookmarking을 써서 bookmark행위를 강조하려했는데 일상적으로 bookmark를 열었다 닫았다 한다고 쓰이기 때문에 명사 bookmark를 그대로 쓰는 것이 단어 길이도 줄어들고 괜찮네요! 반영하겠습니다~
setBookMarkedSentences((prevBookmarkedSentences) => { | ||
if (prevBookmarkedSentences.includes(sentence)) { | ||
return prevBookmarkedSentences.filter((s) => s !== sentence); | ||
} | ||
|
||
return [...prevBookmarkedSentences, sentence]; | ||
}); | ||
}; |
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.
이 부분은 나중에 api 연결할 때 낙관적 업데이트를 적용하면 좋을 것 같습니다~
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.
�오 기존 상태 코드 살리면서 api 연결할 수 있는 방법이겠군요! todo로 적어두겠습니다
<Button | ||
onClick={() => toggleBookmarking(script.enScript)} // 문장 전달 | ||
className={`absolute left-0 -bottom-4 transform -translate-y-1/2 p-1 rounded-full w-8 h-8 flex items-center justify-center transition-opacity ${ | ||
bookMarkedSentences.includes(script.enScript) |
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.
bookMarkedSentences.includes(script.enScript) 이 부분 너무 길어서 변수로 빼고 싶긴 한데 빼도 길 것 같네요. 좋은 방법 없을까요??
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올리긴 했습니다. 나중에 api코드 바꿀때 리팩토링 고민해보면 좋을 것 같아요.
onClick={() => toggleBookmarking(script.enScript)} // 문장 전달 | ||
className={`absolute left-0 -bottom-4 transform -translate-y-1/2 p-1 rounded-full w-8 h-8 flex items-center justify-center transition-opacity ${ | ||
bookMarkedSentences.includes(script.enScript) | ||
? 'bg-green-500 text-white opacity-0 group-hover:opacity-100' |
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.
제가 머지하겠습니다~
📄 Description of the PR
🔧 What has been changed?
세부 구현
📸 Screenshots / GIFs (if applicable)
움짤 로직 설명
: 문장을 북마크하면 파란색 플러스 버튼이 연두색 마이너스 버튼으로 바뀝니다. 북마크한 문장이 사이드바에 추가되었습니다. 사이드바에서 문장을 삭제하니 문장추가버튼이 다시 파란색 플러스 버튼으로 바뀌었습니다.
✅ Checklist