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

[33] 팔로우/언팔로우 기능 구현 및 프로필 페이지의 포스트 목록 api 수정 #73

Merged
merged 12 commits into from
Dec 18, 2019

Conversation

dev-allenk
Copy link
Collaborator

구현사항

  • 팔로우/언팔로우 기능
    • 로그인 된 상태일 때, 팔로우/언팔로우 가능하도록 기능 추가
    • 로그인 안 된 상태일 때, 팔로우 버튼을 누르면 로그인 모달창 띄우는 기능 추가
  • 프로필 페이지에서 해당 유저의 포스트 목록만 보이도록 api 수정
    • 지금은 백엔드에서 보내주는 응답이 동일해요. 백엔드 코드는 수정 필요
  • LoginModalContext작성
    • 로그인모달이 필요한 상태를 관리하는 컨텍스트.
    • LoginContext와는 역할이 다르다고 생각되어 분리했어요.

참고사항

  • Header컴포넌트에 기존 작성되어 있던 함수를 조금 수정했어요.(handleSignin() => toggleSigninModal())
    수정한 의도는, 함수를 사용하는 쪽에서 현재 어떤 상황이고, 그래서 어떤 동작을 해야하는지 명시적으로 알 수 있도록 작성하고자 했습니다.
  • profile-content(게시글 수, 팔로워 수 등)를 받아올 때 필요한 userId가 하드코딩 되어있어요.
    다음주에 [35] 프로필 이미지 및 닉네임 눌렀을 때 해당 유저의 프로필페이지 보여주는 기능 구현 #71 진행하면서 작업 예정입니다.

해결된 이슈 번호

- 기존에는 userId와 토큰을 보내서 서버에서 판단하도록 했지만 토큰의 정보를 클라이언트에 가지고 있기 때문에 불필요하게 됨
- 다른 피쳐에서 styleType='normal'을 디폴트로 수정함
- LoginContext에 needsLoginModal state 추가
  : ProfileInfo에서 발생한 이벤트가 Header 컴포넌트의 상태를 바꿔야하는데
 Header의 상태를 공통 부모로 끌어올리기엔 어색하다고 생각함.
 Header는 그대로 두고 LoginContext에 새로운 상태를 추가하고 그 상태에 따라 Header가 바뀌도록 작성함
- handleSignin/handleSignup함수명을 역할을 좀 더 구체적으로 나타내는 이름으로 수정
  : handleSignin > toggleSigninModal
- toggleSigninModal 함수는 type 파라미터를 명시적으로 받아서 동작할 수 있도록 기능 추가
  : toggleSigninModal('OPEN') > 모달창 열기
- Header쪽 네임스페이스와 겹친다고 생각해서 분리를 시작했으나 디스트럭쳐링으로 해결가능하다는 점을 꺠달음.
- 그래서 굳이 분리할 필요는 없지만 LoginContext와는 역할이 조금 다르다고 생각되어 분리한 채로 유지하기로 함.
@dev-allenk dev-allenk added the new feature New feature or request label Dec 6, 2019
@dev-allenk dev-allenk added this to the iteration 10 : 7/12/2019 milestone Dec 6, 2019
@dev-allenk dev-allenk self-assigned this Dec 6, 2019
Copy link

@devjinius devjinius left a comment

Choose a reason for hiding this comment

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

코드가 깔끔 간결해서 딱히 할 말이 없꾼요 👍 👍

import { IMAGE_BUCKET_URL } from '../../configs';

const Header = () => {
const { inputValue, handleChange, restore } = useInput();
const [clickedSignup, setClickedSignup] = useState(false);
const [clickedSignin, setClickedSignin] = useState(false);
const { loggedIn, profileImage } = useLoginContext();
const {
needsLoginModal: needsSigninModal,
setNeedsLoginModal: setNeedsSigninModal

Choose a reason for hiding this comment

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

context에서는 login이고 view에서는 signin으로 분리하여 사용하는 이유가 있나요??

통일해야 나중에 수정할 때 혼동이 없어서 좋을 것 같은데, 특별한 이유가 있는건가요

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

그 부분을 고민하긴 했어요.
미셸이랑 얘기해봤는데, Header에서는 signup과 함께 쓰이다보니까 짝을 이루는 signin을 사용하는게 자연스럽다고 생각했어요.
login이 일반적으로 사용되는 단어라 익숙하니까 Header를 제외한 부분은 login을 쓰고 Header만 signin을 쓰기로 했답니다.

Choose a reason for hiding this comment

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

이런 내용은 어딘가 기록해두면 좋을 것 같네요!
팀 문서라던지, 주석이라던지, issue일 수 도 있고!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

오.. 그러고보니 그렇네요. 기록남기는게 아직 익숙하지가 않네요 ㅎㅎ
좋은 지적 고마워요!

Comment on lines 27 to 39
const toggleSigninModal = type => {
switch (type) {
case 'OPEN':
setClickedSignin(true);
setNeedsSigninModal(false);
break;
case 'CLOSE':
setClickedSignin(false);
break;
default:
setClickedSignin(!clickedSignin);
break;
}

Choose a reason for hiding this comment

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

이름이 toggle이라 2가지 경우의 수라고 자연스레 생각되는데 내부는 세 가지라서 아주 조금 의아하네요ㅋㅋ
handleSigninModal로 해도 이상할 것 같진 않아요.

참고사항에 적어놓은 의도는 알겠습니다. 제 생각으로는 이름이 함수 내부 코드를 모두 대변하지 못한다는 느낌이 들어서 저라면 handle로 했을 것 같아요.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

그렇네요! toggle이 그런 의미를 담고 있긴하죠 ㅋㅋ
handle로 하는게 더 나을 것 같네요. 좋은 의견 고마워요! :)

};

const handleSignup = () => {
clickedSignup ? setClickedSignup(false) : setClickedSignup(true);
const toggleSignupModal = () => {

Choose a reason for hiding this comment

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

이름이 토글이면 이런걸 자연스레 기대하게 되는데 아까껀 너무 길어서 흠칫ㅋㅋㅋㅋ

Comment on lines 46 to 48
useMemo(() => {
needsSigninModal ? toggleSigninModal('OPEN') : null;
}, [needsSigninModal]);

Choose a reason for hiding this comment

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

요런건 한 줄로 하면 깔끔하고 가독성이 오르지 않나요? 라인 나누기로 컨벤션이 맞춰졌다면 인정!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

eslint 적용중인데 디폴트로 80자에서 줄바꿈 하도록 되어있어서요 ㅎㅎ
너무 짧은 감이 있어서 이런저런 자료를 찾아봤는데 저는 2가지 이유에서 이 설정을 유지하기로 했어요.

  1. 구글 스타일 가이드에서 80자를 제안한다.
  2. 화면을 좌우로 split해서 볼 때 코드를 보기 수월하다.

물론 제 맥북에선 split하면 글자가 잘리지만, 나름 설득력있는 이유라고 생각했어요 ㅎㅎ

const { setNeedsLoginModal } = useLoginModalContext();
const [error, setError] = useState(null);

const handleResponse = res => {

Choose a reason for hiding this comment

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

#72 (comment)

미셸한테도 얘기 했던건데 예상대로 유사한 로직을 중복으로 사용하네요!
같이 얘기해서 핸들러를 하나 만들고 import해서 message, callback 넘겨서 사용하게끔 하면 좋을 것 같네요

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

그렇군요! 새로운 시각으로 코드를 봐주니까 정말 좋네요 ㅎㅎ
에러핸들러 한번 만들어봐야겠어요!

const [data, setData] = useState(null);
const ProfilePage = () => {
//TODO: userId를 전역 context에서 받아서 profile-content api 요청하기
//라우터URL을 nickname으로 표시하기 때문에 prop으로는 userId를 받을 수 없음.

Choose a reason for hiding this comment

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

요건 URL에 user id를 숨기기 위함인가요? 아니면 그냥 하다보니 그렇게 된건가요...?
어찌됐건 지금은 nickname은 고유한 값이 아닌데 key로 사용하려다보니 생기는 문제인 것 같네요ㅠㅠㅠ 잘 해결하시길...

팁으로 저희 서비스의 경우에는 userId를 감추기 위해 url을 비롯한 view에는 hash값을 사용해요.
예를들면 user/we292erv 이런식으로요. 그러면 hash도 고유한 값이니까 hash로 userdata를 가져오는 api를 만들고 거기서 얻은 정보를 view에서 이용해요.

서버에서는 userId, view에서는 hash 요렇게 사용합니다

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

음.. 2가지 원인에 의해 이런 상황이 되었어요.

  1. 백엔드에서 userId를 priority key로 사용함.
  2. 프론트쪽 URL에는 nickname으로 표시를 하고 싶음(그냥 팀 내 의견)

context쓰면 가능할 것 같아서.. 앱 성능은 어떨지 모르겠지만 큰 문제는 아닐 것 같아요!
hash도 좋은 생각인데 sns서비스를 만들다보니까 사용자 입장에서는 URL이 친숙하게 보이는 것도 중요하다고 생각해요 :)

@dev-allenk
Copy link
Collaborator Author

진! 피드백 정말정말 고마워요 :)
많은 도움이 되었어요!
앞으로도 종종 부탁해요 ㅋㅋ

- Header컴포넌트
 : toggleSigninModal > handleSigninModal
- 단순한 토글이라기보다는 인자값에 따라 핸들링 하는 의미를 표현
@dev-allenk dev-allenk merged commit bbdd804 into develop Dec 18, 2019
Copy link
Contributor

@leehwarang leehwarang left a comment

Choose a reason for hiding this comment

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

구두로 이야기 나눴던 부분 적어둘게요!

LoginModalContext

  • 웹페이지 내에서 비로그인한 사용자가 회원가입이 필요한 기능을 클릭할 때가 있음. -> 따라서 그럴 때 LoginModal 을 띄워야함 -> 현재는 Header 컴포넌트에서만 LoginModal 을 띄우도록 설계 되어 있기 때문에, 다른 컴포넌트들에서도 LoginModal 을 띄울 수 있어야함
    • 엘런의 해결 방식: LoginModalContext 를 만들었고, 모든 컴포넌트에서 setNeedsLoginModal(setState)를 설정할 수 있게 한다. Header 에서 needsSigninModal의 값 변화를 관찰하고 있고, true 로 변경되면 handleSigninModal 을 띄움. 그렇기 때문에 Header의 handleSignin 에서 ‘OPEN’시에 clikedSignin은 true 로 하고, needSigninModal 은 false 로 바꿔줄 필요가 있는 것.
    • 나의 생각: 이미 LoginContext로 사용자가 로그인한지 안했는지는 전역에서 판단할 수 있으니, 만약 웹페이지 내에서 비로그인한 사용자가 회원가입이 필요한 기능을 클릭할 때가 있음. 이런 기능이 필요한 컴포넌트에서는 그냥 Header의 handleSigninModal 함수를 불러와서 사용하면 되지 않나? 지금 방식은 Header에서 모달 열고 닫힐 때, need 어쩌고 까지 체크해줘야 하니 읽기 좀 힘들었고, Context가 많아지는게 괜찮은지? 고민이 들었음.

Comment on lines +46 to +49
useMemo(() => {
needsSigninModal ? handleSigninModal('OPEN') : null;
}, [needsSigninModal]);

Copy link
Contributor

Choose a reason for hiding this comment

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

여기서 useEffect 말고 왜 useMemo 사용했는지 이야기해주면 감사링...
성능상 어떻게 이득이 되는지 10번 정도 더 들어야 할 듯 ㅎㅎ

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

useMemo에서 state를 변경하면 바로 바뀌구요, useEffect에서 state를 변경하면 나중에 바뀌어요.
단순히 이 차이 때문에 useMemo를 사용했는데, 좀 알아보니 hooks의 목적에는 안맞는 것 같네요.
useMemo에는 pure function만 들어가는게 올바른 사용법이긴 한듯!
(하지만 impure function을 사용해도 별 문제 없는 것 같아서 자꾸 쓰게됨..)

@@ -13,13 +13,13 @@ import {
} from '../../configs';
import { throttle } from '../../utils/utils';

const PostContainer = ({ headerOn, api }) => {
const PostContainer = ({ headerOn, api, query: writerId = '' }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

props로 넘겨 받은 query라는 key 값을 PostContainer 에서 사용시 writerId 라는 걸로 사용하고, 값이 없으면 ''로 초기화 한다는 코드죠? 근데 PostContainer 는 여러개의 post를 담는데, 특정한 writerId 를 어떻게 보내주나요?

Copy link
Contributor

Choose a reason for hiding this comment

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

답) mainPage 에서 사용하는 PostContainer 에서는 writerId 가 항상 빈 문자열이고, 특정 사용자 프로필 페이지에서 사용하는 PostContainer 에서는 writerId가 들어감

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

다른 api랑 조금 다르게 동작하는 api라서 이해가 어려울 수도 있겠네요. 생각하지 못했어요.
api에 대한 설명을 주석으로 남겨두는 것도 괜찮겠네요.

Comment on lines +49 to +52
const res = await fetch(`${WEB_SERVER_URL}/user/follow/${userId}`, {
method: `${isFollowing ? 'DELETE' : 'POST'}`,
credentials: 'include'
});
Copy link
Contributor

Choose a reason for hiding this comment

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

내가 이 userId 를 팔로우하고 있는지 파악하기 위해서 토큰 보내 확인하는군요!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New feature or request
Projects
Frontend Task Managing
  
Awaiting triage
Development

Successfully merging this pull request may close these issues.

[33] 프로필 페이지에 필요한 기능 구현(팔로우, 포스트 목록)
3 participants