Skip to content

Conversation

@eundeok9
Copy link

@eundeok9 eundeok9 commented May 14, 2022

3주차_1
3주차_2

Copy link

@devHudi devHudi left a comment

Choose a reason for hiding this comment

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

전체적으로 컴포넌트 설계가 깔끔한 것 같아 놀랐습니다.
몇가지 수정하면 좋을 만한 점을 코멘트 달아드렸어요. 반영해주셔도 좋고, 참고만 해주셔도 좋습니다 😁

align-items: center;
cursor: pointer;
text-align: center;
color: ${(props) => (props.fColor ? props.fColor : "black")};
Copy link

Choose a reason for hiding this comment

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

단체 톡방에 말씀드린대로 short circuit evaluation 을 통해 축약해볼 수 있겠네요 🙂

Comment on lines +4 to +38
const StyledButton = styled.div`
display: flex;
justify-content: center;
align-items: center;
cursor: pointer;
text-align: center;
color: ${(props) => (props.fColor ? props.fColor : "black")};
font-size: ${(props) => props.fSize};
height: ${(props) => props.btnHeight};
border: 1px solid transparent;
border-color: ${(props) => props.borderColor};
border-radius: ${(props) => props.borderRadius};
background-color: ${(props) => props.bgColor};
padding: ${(props) => props.btnPadding};
margin: ${(props) => props.btnMargin};
`;

const Button = (props) => {
return (
<StyledButton
fColor={props.fColor}
fSize={props.fSize}
btnHeight={props.btnHeight}
borderColor={props.borderColor}
borderRadius={props.borderRadius}
bgColor={props.bgColor}
btnPadding={props.btnPadding}
btnMargin={props.btnMargin}
>
{props.children}
</StyledButton>
);
};

export default Button;
Copy link

Choose a reason for hiding this comment

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

Suggested change
const StyledButton = styled.div`
display: flex;
justify-content: center;
align-items: center;
cursor: pointer;
text-align: center;
color: ${(props) => (props.fColor ? props.fColor : "black")};
font-size: ${(props) => props.fSize};
height: ${(props) => props.btnHeight};
border: 1px solid transparent;
border-color: ${(props) => props.borderColor};
border-radius: ${(props) => props.borderRadius};
background-color: ${(props) => props.bgColor};
padding: ${(props) => props.btnPadding};
margin: ${(props) => props.btnMargin};
`;
const Button = (props) => {
return (
<StyledButton
fColor={props.fColor}
fSize={props.fSize}
btnHeight={props.btnHeight}
borderColor={props.borderColor}
borderRadius={props.borderRadius}
bgColor={props.bgColor}
btnPadding={props.btnPadding}
btnMargin={props.btnMargin}
>
{props.children}
</StyledButton>
);
};
export default Button;
const Button = styled.div`
display: flex;
justify-content: center;
align-items: center;
cursor: pointer;
text-align: center;
color: ${(props) => (props.fColor ? props.fColor : "black")};
font-size: ${(props) => props.fSize};
height: ${(props) => props.btnHeight};
border: 1px solid transparent;
border-color: ${(props) => props.borderColor};
border-radius: ${(props) => props.borderRadius};
background-color: ${(props) => props.bgColor};
padding: ${(props) => props.btnPadding};
margin: ${(props) => props.btnMargin};
`;
export default Button;

Button 컴포넌트가 props 를 그대로 StyledButton 에 전달하는 역할만 수행하고 있는데요, 위와 같이 축약해볼 수 있을 것 같아요. 😎

Comment on lines +4 to +19
const StyledCardContainer = styled.div`
max-width: 1300px;
height: 1250px;
border: 0px;
background-color: #f9f9f9;
display: flex;
flex-wrap: wrap;
justify-content: center;
align-items: center;
`;

const CardContainer = (props) => {
return <StyledCardContainer>{props.children}</StyledCardContainer>;
};

export default CardContainer;
Copy link

Choose a reason for hiding this comment

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

위와 마찬가지로 props 를 그대로 전달하는 컴포넌트는 축약해볼 수 있을 것 같아요. 나머지 컴포넌트들도 포함해서요! 😄

Comment on lines +3 to +8
const GlobalStyle = createGlobalStyle`
*{
margin: 0;
font-family:-apple-system,BlinkMacSystemFont,"Segoe UI",Roboto,"Helvetica Neue",Arial,"Noto Sans",sans-serif,"Apple Color Emoji","Segoe UI Emoji","Segoe UI Symbol","Noto Color Emoji";
}
`;
Copy link

Choose a reason for hiding this comment

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

👍👍

Comment on lines +64 to +82
const Header = (props) => {
return (
<HeaderWrapper>
<HeaderContainer>
<LogoWrapper>
<LogoImage>
<HiOutlineCamera size="24" color="white" />
</LogoImage>
<LogoText>Album</LogoText>
</LogoWrapper>
<HeaderButtonWrapper>
<HiOutlineMenu size="30" color="rgba(255,255,255,.5)" />
</HeaderButtonWrapper>
</HeaderContainer>
</HeaderWrapper>
);
};

export default Header;
Copy link

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 +21
fcolor={props.color}
fSize={props.fontSize}
Copy link

Choose a reason for hiding this comment

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

다른 개발자가 fcolorfSize props의 네이밍을 보고 바로 font 라는 단어를 유추할 수 있을까요? 좋은 네이밍은 무엇일까요? 어떻게 하면 좀 더 명확하게 네이밍하고 커뮤니케이션 비용을 줄일 수 있을까요? 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants