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

[GGFE-186] 메뉴바 티어 색상 수정 #939

Merged
merged 7 commits into from
Aug 24, 2023

Conversation

parksangmin1543
Copy link
Contributor

📌 개요

  • 메뉴바에서 색상변경안되는 버그 수정

💻 작업사항

  • Selector에서 profileState사용 중이지만 메뉴바에서 profileState사용시 다른 사람 프로필접근시 바뀌는 버그가 있어 사용중이지 않아 Selector사용안하는 방향으로 수정했습니다.
  • 좋은 방법 알려주시면 수정하겠습니다!

✅ 변경로직

@parksangmin1543 parksangmin1543 changed the title [Bug] [GGFE-186] 메뉴바 티어 색상 수정 [GGFE-186] 메뉴바 티어 색상 수정 Aug 17, 2023
Copy link
Contributor

@PHJoon PHJoon left a comment

Choose a reason for hiding this comment

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

다른 사람 프로필이랑 연동되는 문제는 어쩔 수 없군요 ㅠ 저는 user에 정보를 추가로 받아오는 방식말고는 생각이 안나네요 🫥 고생하셨습니다!! 👍

Comment on lines 48 to 52
const getProfile = useMockAxiosGet<any>({
url: `users/intraId`,
setState: setProfile,
setState: (data) => {
setProfile(data);
},
Copy link
Member

Choose a reason for hiding this comment

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

제가 제대로 이해한건지 모르겠는데, 티어가 추가되기 전부터 여기서 userState 말고 또 다른 유저 데이터를 쓰는 이유가 userState 안에 없는 level을 쓰기 위해서가 맞나요...?
만약에 그 이유가 맞다면 차라리 userState로 받아오는 값에 tierName이랑 level을 받아올 수 있는지 물어보고 userState의 값으로 메뉴바의 정보를 넣어주는게 오히려 더 좋을 것 같아요.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

네 그게 제일 좋은 방법일것같습니다. user 정보에 tierName level정보를 담아달라고 연락드리겠습니다!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

user에 tierName, level로 변경해서 다시 올렸습니다!

@hyobb109
Copy link
Member

변경사항 확인했습니다! 이제 유저 정보에 필요한 것들은 다 넣은 것 같네요..! 고생하셨습니다👍

Copy link
Member

@yoouyeon yoouyeon left a comment

Choose a reason for hiding this comment

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

문제없이 잘 되네요! 처음부터 user 정보에 level을 추가했으면 되었을 것을 빙빙 돌아온 느낌이네요... 😇
수고하셨습니다!! 👍

Comment on lines 71 to 38
const index = tierList.findIndex((tier) => tier[0] === profile.tierName[0]);
const index = tierList.findIndex((tier) => tier[0] === user.tierName[0]);
const tierId = tierColor[index];
const findTierIndex =
tierId === 'none' ? styles.tierId : styles['tierId' + tierId];
Copy link
Member

Choose a reason for hiding this comment

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

유저 정보의 한글 티어 이름의 첫번째 글자를 이용해서 인덱스를 찾고, 해당 인덱스에 맞는 영어 티어 이름으로 바꾸는 것 같은데 이렇게 한글 → 숫자 → 영어 변환을 하게 되니까 변수도 많아지고 흐름이 한번에 이해가 잘 안되는 것 같아요

  const tierColor = {
    '손': 'none',
    '빨': 'red',
    '노': 'yellow',
    '초': 'green',
    ...
  }

이런식으로 key를 이용해서 영어 티어 이름을 한번에 찾아가는건 어떨까요??

Comment on lines 62 to 64
<div className={`${styles.tierId} ${findTierIndex}`}>
{profile.tierName}
{user.tierName}
</div>
Copy link
Member

Choose a reason for hiding this comment

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

여기서 findTierIndex 가 정확히 어떤 역할을 하는건지 잘 모르겠습니다..!

const findTierIndex = tierId === 'none' ? styles.tierId : styles['tierId' + tierId];

'tierId'라는 문자열을 색깔이 들어가는 클래스명에 붙여주기 위해서인가요??
지금 코드로는 tierId가 'none'인 경우에는 styles.tierId 가 중복으로 들어가게 되는 것 같아서 아래처럼 삼항연산자 수정해줘야 할 것 같습니다..

<div className={`${styles.tierId} ${tierId === 'none' ? '' : styles['tierId' + tierId]}`}>

그리고 개인적인 의견일수도 있지만 findTierIndex라는 변수 이름이랑 Id에 맞게(근데 왜 색깔 이름을 가리키는 변수명이 tierId인가요..? 😵‍💫) 클래스명 지어주는 내용이랑 좀 어울리지 않는 것 같아요. 좀 더 의도에 맞는 변수 이름으로 바꾸거나 아니면 변수에 담지 않고 위 코드처럼 바로 삼항연산자를 넣는 방식이 어떨까 싶습니다..

근데 왜 색깔 이름에 tierId라는 prefix를 붙여주는건가요?? 간단히 테스트 해 봤을 때에는 없어도 문제는 없어 보여서요!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tierid를 붙인 이유는 색깔이름이 아닌 숫자로 넣었을때 1,2,3만 표시가 되서 구분을 위해 있던 부분이라 수정해서 올리겠습니다.

Copy link
Member

@yoouyeon yoouyeon left a comment

Choose a reason for hiding this comment

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

훨씬 보기 편해졌네요 👍 수고하셨습니다!!!

검: 'black',
무: 'rainbow',
};
console.log(tierColor[user.tierName[0]]);
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
Contributor

@PHJoon PHJoon 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 15
import { HeaderContextState, HeaderContext } from '../HeaderContext';
import { MainMenu, AdminMenu } from './MenuBarElement';

Copy link
Contributor

Choose a reason for hiding this comment

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

여기 지워주시면 될거같아요! 위에 절대경로로 import 되어 있어서 겹치네요!

@parksangmin1543 parksangmin1543 merged commit aaaac52 into main Aug 24, 2023
@PHJoon PHJoon deleted the GGFE-186-Bug-티어메뉴 branch September 4, 2023 06:30
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.

4 participants