Skip to content
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-fe: 공통 컴포넌트 SideBar 구현 #271

Merged
merged 10 commits into from
Aug 6, 2024
Merged

Conversation

lurgi
Copy link
Contributor

@lurgi lurgi commented Aug 5, 2024

작업 내용

참고 사항

관련 이슈

@lurgi lurgi self-assigned this Aug 5, 2024
@lurgi lurgi added this to the 스프린트 2.0 milestone Aug 5, 2024
Copy link
Contributor

github-actions bot commented Aug 5, 2024

1722864361.664549

Copy link
Contributor

@llqqssttyy llqqssttyy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

고생하셨습니다 러기~ 당장 수정이 필요한 부분은 보이지 않아 approve 드립니다.

{options.map(({ text, isSelected }, index) => (
// eslint-disable-next-line react/no-array-index-key
<Accordion.ListItem key={index}>
<S.ContentButton isSelected={isSelected}>{text}</S.ContentButton>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 버튼들을 눌러서 페이지를 이동해야 하니 NavigateButton 정도의 이름도 괜찮을 것 같아요.

Copy link
Contributor

@seongjinme seongjinme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

러기 고생하셨어요! 스타일 상으로 한 가지 제안드릴 부분이 있어 코멘트 남겨드렸어요. 체크해보시고 적용 여부를 자유롭게 결정해주시면 될 것 같습니다.

Comment on lines +13 to +18
// TODO: 현재 아코디언의 오픈값을 True로 설정합니다. 추후에 아코디언이 추가될 경우 변경이 필요합니다.
const [isOpen] = useState(true);

const toggleAccordion = () => {
// setIsOpen(!isOpen);
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

요 부분은 추후에 수정 필요한 부분으로 이해하고 넘어갈게요!

Comment on lines +20 to +22
<Accordion.ListItem key={index}>
<S.NavButton isSelected={isSelected}>{text}</S.NavButton>
</Accordion.ListItem>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accordion.ListItem 컴포넌트 요소를 따로 빼고, 그 안에 button 요소를 넣으셨네요. 이런 경우에 해당 리스트(<li>) 항목에 대한 bullet point 표식은 Accordion.ListItem 보다는 button 요소에 삽입했다면 어땠을까 싶습니다. 현재는 isSelected prop에 의한 스타일 변경 사항이 button 요소에만 적용되고 bullet point에는 적용되지 않아서, 실제 Storybook으로 체크해보면 조금 어색하게 느껴지거든요.

Accordion.ListItem에 삽입되어 있는 아래의 스타일 코드를 S.NavButton으로 대신 적용하는 아이디어를 한 번 검토해주시면 좋겠습니다.

  padding: 0.4rem 0rem;
  &::before {
    content: '•';
    width: 1rem;
    aspect-ratio: 1/1;
    margin-right: 0.8rem;
  }

Copy link
Contributor

@seongjinme seongjinme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

수정된 코드와 스토리북 모두 확인했습니다. 적용해주셔서 감사드려요! 🙏

@lurgi lurgi merged commit 7f4ebc1 into fe/develop Aug 6, 2024
8 checks passed
@lurgi lurgi deleted the fe-263-COM_SIDEBAR branch August 6, 2024 06:26
lurgi added a commit that referenced this pull request Aug 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 완료
Development

Successfully merging this pull request may close these issues.

3 participants