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

너가소개서/팀원소개서/마이페이지 Header #32

Merged
merged 18 commits into from
Jan 11, 2022
Merged

Conversation

100Gyeon
Copy link
Member

⛓ Related Issues

📋 작업 내용

  • Header 구조화 및 스타일링

📌 PR Point

  • 모든 a 태그의 text-decoration을 없애는 게 좋을 것 같아서 GlobalStyle 추가했습니다.
  • 더 좋은 구현 방법이 있다면 리뷰 남겨주세요~!

👀 스크린샷 / GIF / 링크

@100Gyeon 100Gyeon added feature 🎄 기능 개발 layout 🍃 레이아웃 개발 labels Jan 10, 2022
@100Gyeon 100Gyeon self-assigned this Jan 10, 2022
@100Gyeon 100Gyeon added this to In progress in 팀원소개서 via automation Jan 10, 2022
@SeojinSeojin
Copy link
Member

울 웹쁜이 고생많았어 ! 여기서 미리보기로 보면서 쉬어~

@@ -1,2 +1,2 @@
export { default as Logo } from './logo.svg';
export { default as imgLogo } from './img_logo.svg';
Copy link
Member

Choose a reason for hiding this comment

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

💟✨💓

Copy link
Member

Choose a reason for hiding this comment

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

바꿔줬구낭 ㅎㅎ고마워~

Copy link
Member

@Hyoin-Kim Hyoin-Kim left a comment

Choose a reason for hiding this comment

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

수고많았어 ~ 코드 야무지게 잘짰넹

@@ -1,2 +1,2 @@
export { default as Logo } from './logo.svg';
export { default as imgLogo } from './img_logo.svg';
Copy link
Member

Choose a reason for hiding this comment

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

바꿔줬구낭 ㅎㅎ고마워~

Comment on lines 18 to 23
& > a {
margin-right: 16px;
padding-bottom: 10px;
color: ${COLOR.GRAY_4};
cursor: pointer;
}
Copy link
Member

Choose a reason for hiding this comment

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

이 태그 어디에서 쓰는거징

Copy link
Member Author

Choose a reason for hiding this comment

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

이거 NavLink 부분에서 a 태그로 쓰여!

Copy link
Member

Choose a reason for hiding this comment

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

NavLink도 a구나!!

Copy link
Member

@SeojinSeojin SeojinSeojin left a comment

Choose a reason for hiding this comment

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

우리가 리액트와 스타일드 컴포넌트를 사용하고 있는데 엘리먼트의 id나 style에 바로 접근하는건 막 권장되지는 않을 것이야..!!

내가 다른 방법을 제안해볼게!!

const [currentTab, setCurrentTab] = useState(pageName.match("\/(?:.(?!\/))+$")[0]);

const tabs = [ { name:'너가소개서', href="home"}, {name:'팀원소개서', href="team", ...}]

...

tabs.map(tab => 
<NavLink 
	to={`/home/${tab.href}`} 
	onClick={()=>setSelectedTab(tab.href)} 
	isSelected={selectedTab === tab.href}
>
	{tab.name}
</NavLink>)

이런 방식은 어떨까!

Comment on lines 29 to 30
prevPage.style.color = `${COLOR.GRAY_4}`;
prevPage.style.borderBottom = `none`;
Copy link
Member

Choose a reason for hiding this comment

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

바로 style에 접근하는 것보다 Styled Component의 프롭으로 isCurrent이라는 값을 주고 그 값에 따라 스타일을 바꿔주는건 어떨까?

Copy link
Member Author

Choose a reason for hiding this comment

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

좋아좋아 나도 직접 접근하는 부분이 마음에 걸려서 수정하고 싶었어😂 방법 참고해서 바꿔볼게!

Copy link
Member

Choose a reason for hiding this comment

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

나도 스타일컴포넌트로 넘겨주는건 어떨지 고민하다가 나도 저렇게 코드짤것같아서 그대로 놔뒀는데,,,ㅎㅎㅎㅎㅎㅎㅎㅎㅎㅎㅎ

Copy link
Member

Choose a reason for hiding this comment

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

친구들이 말한거랑 같은 맥락으로 요런 식으로 어때!!

const StNavLink = styled(NavLInk)<{ isSelected: boolean }>`
` 

Copy link
Member Author

Choose a reason for hiding this comment

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

그 방법도 좋지 ✔

const location = useLocation();
const pageName = location.pathname;

const [prev, setPrev] = useState('');
Copy link
Member

Choose a reason for hiding this comment

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

이 친구는 용도가 먼가요?

current라는 값만 있어도 스타일 지정하는 데엔 문제 없을 것 같아요..!!

Copy link
Member Author

Choose a reason for hiding this comment

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

클릭한 부분에 스타일 적용해 주고, 아닌 부분에 스타일 빼주고 싶어서 나눠서 생각했는데 이것도 같이 수정해야겠다 !!

@SeojinSeojin
Copy link
Member

울 웹쁜이 고생많았어 ! 여기서 미리보기로 보면서 쉬어~

@SeojinSeojin
Copy link
Member

울 웹쁜이 고생많았어 ! 여기서 미리보기로 보면서 쉬어~

Copy link
Member

@SeojinSeojin SeojinSeojin left a comment

Choose a reason for hiding this comment

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

수과셨습니다!!!✨✨✨✨

key={tab.name}
to={tab.href}
onClick={() => setCurrentTab(tab.href)}
className={currentTab === tab.href ? 'current' : ''}
Copy link
Member

Choose a reason for hiding this comment

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

styled component가 자동으로 className을 만들어주니까 우리 단에서 클래스이름을 부여하는 것이 좋지 않을 수 있을 것 같아..!!!

currentTab === tab.href 이 친구를 isSelected라는 prop으로 넘기면 좋을 것 같다!!

Copy link
Member Author

Choose a reason for hiding this comment

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

좋은 방법이야 고마워 서진아 😉

import { StHeaderWrapper } from './style';
import { imgLogo } from '@assets/images';

function Header() {
Copy link
Member

Choose a reason for hiding this comment

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

이 친구 Home에서 쓰이니까 HomeHeader 해주면 어떨까?

image
이 친구와 이름이 헷갈릴 수 있을듯..!!

Copy link
Member Author

@100Gyeon 100Gyeon Jan 10, 2022

Choose a reason for hiding this comment

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

다른 곳에도 헤더가 있구나 ㅠㅠ 수정했어!

@SeojinSeojin
Copy link
Member

울 웹쁜이 고생많았어 ! 여기서 미리보기로 보면서 쉬어~

@SeojinSeojin
Copy link
Member

울 웹쁜이 고생많았어 ! 여기서 미리보기로 보면서 쉬어~

@SeojinSeojin
Copy link
Member

울 웹쁜이 고생많았어 ! 여기서 미리보기로 보면서 쉬어~

@SeojinSeojin
Copy link
Member

울 웹쁜이 고생많았어 ! 여기서 미리보기로 보면서 쉬어~

Copy link
Member

@SeojinSeojin SeojinSeojin left a comment

Choose a reason for hiding this comment

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

수고하셨습니다!!!👍

<StNavLink
key={tab.name}
to={tab.href}
onClick={() => setCurrentTab(tab.href)}
Copy link
Member

Choose a reason for hiding this comment

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

위의 useEffect에서 location이 바뀔 때마다 currentTab이 바뀌게 해뒀는데

그럼 이 부분은 없어도 괜찮지 않을까?

이 친구가 눌리면 location이 tab.href로 바뀌니까!

Copy link
Member Author

Choose a reason for hiding this comment

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

그러네~! 나중에 useEffect 추가하면서 아래 코드까지 고려를 못했다 고마워 👍🏻

to={tab.href}
onClick={() => setCurrentTab(tab.href)}
current={currentTab === tab.href ? 'current' : ''}
>
Copy link
Member

Choose a reason for hiding this comment

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

와 진짜 멋지게 짰다 !!!!!!!

@SeojinSeojin
Copy link
Member

울 웹쁜이 고생많았어 ! 여기서 미리보기로 보면서 쉬어~

@SeojinSeojin
Copy link
Member

울 웹쁜이 고생많았어 ! 여기서 미리보기로 보면서 쉬어~

@SeojinSeojin
Copy link
Member

울 웹쁜이 고생많았어 ! 여기서 미리보기로 보면서 쉬어~

@100Gyeon 100Gyeon merged commit af94a9c into dev Jan 11, 2022
팀원소개서 automation moved this from In progress to Done Jan 11, 2022
@100Gyeon 100Gyeon deleted the feat/#28 branch January 11, 2022 11:58
@100Gyeon 100Gyeon added this to Done in 웹 칸반 보드 Jan 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature 🎄 기능 개발 layout 🍃 레이아웃 개발
Development

Successfully merging this pull request may close these issues.

너가소개서/팀원소개서/마이페이지 Header
4 participants