-
Notifications
You must be signed in to change notification settings - Fork 7
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
'내 템플릿' 페이지의 태그 필터 박스 위치/디자인 리팩토링 #516
Conversation
…-teams/2024-code-zap into refactor/510-tagbox-redesign
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.
코멘트 남겼습니당~
@@ -11,7 +11,9 @@ export const TagButtonWrapper = styled.button<{ isFocused: boolean }>` | |||
justify-content: center; | |||
|
|||
box-sizing: border-box; | |||
padding: 0.25rem 0.75rem; | |||
height: 1.75rem; | |||
margin: ${({ isFocused }) => (isFocused ? '0' : '.0625rem')}; |
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.
여기서 margin을 바꾸는 이유가 궁금해요! border 때문인가 싶은데 그럼 컴포넌트가 조금씩 움직이는거 같아서요
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.
저도 궁금합니다!
저는 isFocused
시 border가 2px로 바뀌는 것 때문인가? 생각했어요. 그런데 그런 이유라면 사실 border 를 그냥 선택되지 않았을 때와 동일하게 1px 로 두어도 괜찮을 것 같아요!
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.
원래는 border이 생김으로써 심하게 움직였기 때문에 margin수정하게 되었습니다.
그럼에도 조금씩 움직이는 이유는 Windows Chrome 버전에서 버그가 있다고 대충 알고 있는데요, 테두리가 1px로 설정해도 0.64px이 되는 문제입니다.
헤인의 의견대로 리팩토링 하였습니다.
const unselectedTags = deselectedTags.concat( | ||
tags.filter( | ||
(tag) => !selectedTagIds.includes(tag.id) && !deselectedTags.some((deselectedTag) => deselectedTag.id === tag.id), | ||
), | ||
); |
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.
deselectedTags와 unselectedTags가 헷갈렸었는데 네이밍만 살짝 고민해보면 좋을 것 같아요~!!
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.
deselected는 "선택이 해제된", unselected는 "선택되지 않은" 이라는 의미라고 알고 있어서 이렇게 네이밍 하였습니다!! 저는 어느정도 시멘틱 하다고 생각합니닷..!
<Flex> | ||
{tags.length !== 0 && ( | ||
<TagFilterMenu tags={tags} selectedTagIds={selectedTagIds} onSelectTags={handleTagMenuClick} /> | ||
)} | ||
</Flex> |
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.
TagFilterMenu 컴포넌트의 TagButtonsContainer에도 flex가 걸려있어서 여기서 Flex 컴포넌트를 한번 더 안 감싸도 될 것 같아융
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.
2024-08-21.1.17.52.mov
지금 여전히 화면 변경에 따라서는 대응이 되지 않고 있네요🥲
대응하기 어렵다면, 차리리 chevron 을 항상 두는 것은 어떨까요? 디자인 상 예쁘지 않지만 그래도 화면이 줄어들었을 때 반드시 새로고침해야만 나오는 것은 해결할 수 있을 것 같아요..!
@@ -9,24 +12,68 @@ interface Props { | |||
} | |||
|
|||
const TagFilterMenu = ({ tags, selectedTagIds, onSelectTags }: Props) => { | |||
const [deselectedTags, setDeselectedTags] = useState<Tag[]>([]); | |||
const [isExpanded, setIsExpanded] = useState(false); |
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.
이건 아주 소소한 제안사항입니다만 isExpanded,
보다는 isOpen
과 같은 네이밍은 어떤가요??
+ useToggle
hook을 사용해봐도 좋을 것 같아요!
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.
고생했습니다~!
⚡️ 관련 이슈
#510
📍주요 변경 사항
1. 태그 필터 박스의 위치 변경 : 검색창 하단
2. 태그 wrap 정책 지정 : 접기/펼치기
3. 태그 정렬