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

[Team15 - FE] 2주차 PR #66

Merged
merged 110 commits into from
May 3, 2021
Merged

Conversation

Seohyoung
Copy link
Collaborator

추가 진행 필요사항:

  • Modal을 createPortal을 사용해 렌더링했는데, 내부 캐로셀이 앞뒤로 움직이면서 리렌더링되는 이슈가 발생함. 원인 확인 후 수정예정
  • Modal에서 사용하는 캐로셀이 메인에서 사용하는 캐로셀과 세부 기능이 일부 다른 관계로, 별도로 캐로셀 컴포넌트를 만들었으나 재사용하지 않음. 모달 내부 캐로셀 재사용 필요

📌 질문 1--------------------------------------------------------
화면 기록 2021-04-30 오후 6 54 44 mov

구현 중 이벤트 동작유지가 왜 안되는지 궁금합니다.

공부한 지식
OnMouseOver,Out: 이벤트 동작 시 자식까지 영향
OnMouseLeave,Enter: 이벤트 동작 시 자기 자신한테만

문제상황
안에 을 자식으로 넣고 마우스 호버 이벤트를 넣었는데
Span에 MouseOver를하면 자식인 HeaderDrop에 Mouse가 올라갈 때도 displayDrop이벤트가 유지되어야 한다고 생각하는데 onMouseOver, onMouseEnter두가지를 다 w적용해보아도 마우스가 HeaderDrop으로 내려갔을 때 displyDrop이벤트가 풀리는 점이 의문이었습니다.

HeaderLeft코드링크

📌 질문 2--------------------------------------------------------

화면 기록 2021-04-30 오후 7 14 00 mov

Details(모달창)의 Carousel 이벤트 구현 시 캐로셀이 끝나면 Home의 data가 리렌더링되며 모달창이 꺼지는 문제가 있습니다. 제가 캐로셀 라이브러리를 분리했지만 완벽히 재사용성이 보장되지 않아 코드를 중복사용하면서 상태값이 공유되는 문제인듯 해, 추가로 원인을 확인해보기는 하겠으나 설계 상의 문제인지 궁금합니다..!

[참고 코드] (https://github.com/jihye-woo/sidedish/blob/dev-FE/frontend/src/components/organisms/Carousel/index.jsx)

📌 질문 3--------------------------------------------------------

  • 다른 팀 리뷰에서 createPortal는 굳이 사용하지 않아도 된다고 언급해주셨는데, 일반적으로는 현업에서 모달 작업 시 createPortal보다는 일반적인 render function을 사용하는지 궁금합니다!

Seohyoung and others added 30 commits April 19, 2021 22:40
 - CRA를 설치했습니다
[#1] feat : mediumCard index.jsx prop 추가
 - node sass를 추가했습니다
 - atoms, molecules, images 등 파일 구조 수정했습니다
- reset scss 추가
- 기본 글꼴 추가
 - atoms의 button, icon, tag component를 재사용 가능한 형태로 생성했습니다.
- LargeCard 구조설정
- MediumCard 구조 설정
 - 파일 명을 sidedish -> frontend로 바꾸는 과정에서 폴더 구성이 꼬여서 수정
[#5] chore : 잘못 생성된 파일 수정
 - 신규 컴포넌트 생성: maindish, more organisms
- util/loadData 생성 : fetch로 data가져오기
- util/url 생성: 기본 url주소 변수 설정
 - 메인요리에 medium카드를 사용하기 위해 medium카드 일부 속성을 수정했습니다.
 - fetch기능을 분리하기 위해 util-useFetch컴포넌트를 생성
- HeaderLeft 구조 생성
- HeaderRight 구조 생성
- BestDish 구조 생성 중
ink-0 and others added 22 commits April 29, 2021 18:45
 - Other Card의 레이아웃 완성
- portal을 적용해서 기존 캐러셀 컴포넌트를 재활용할 수 없는 관계로 캐로셀을 중복해서 사용함
@MJbae MJbae added the review-FE FE 리뷰 label Apr 30, 2021
@crongro
Copy link

crongro commented May 3, 2021

  • 다른 팀 리뷰에서 createPortal는 굳이 사용하지 않아도 된다고 언급해주셨는데, 일반적으로는 현업에서 모달 작업 시 createPortal보다는 일반적인 render function을 사용하는지 궁금합니다!

리액트가 렌더링할때 예외적인 처리를 하는 것임으로 안쓰는게 더 좋긴하겠죠.
하지만 modal 을 구현하는 상황등에서는 유용하게 쓰이는 것 같습니다.

@crongro
Copy link

crongro commented May 3, 2021

Details(모달창)의 Carousel 이벤트 구현 시 캐로셀이 끝나면 Home의 data가 리렌더링되며 모달창이 꺼지는 문제가 있습니다. 제가 캐로셀 라이브러리를 분리했지만 완벽히 재사용성이 보장되지 않아 코드를 중복사용하면서 상태값이 공유되는 문제인듯 해, 추가로 원인을 확인해보기는 하겠으나 설계 상의 문제인지 궁금합니다..!

어떤 state가 변경되었는지를 디버깅해봐야겠네요.
리액트는 상태관리 관점에서 문제를 해결해야 하는 경우가 많아요.

@crongro crongro self-assigned this May 3, 2021
@crongro crongro self-requested a review May 3, 2021 01:01
Copy link

@crongro crongro left a comment

Choose a reason for hiding this comment

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

jsx표현에서 && 또는 3항연산자를 사용한 표현법을 활용하면 더 좋겠네요.
수고하셨어요.

<WrapDiv>
<Input></Input>
<Icon
_type="SearchIcon"
Copy link

Choose a reason for hiding this comment

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

underbar를 사용할때가 따로 있는것인가요? 보통은 private 속성/메서드를 의미합니다.

font-size: 16px;
`;

const SearchBar = ({ children, ...props }) => {
Copy link

Choose a reason for hiding this comment

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

{ children, ...props }
이값은 컴포넌트 내부에서 사용하지 않는 듯?

Comment on lines +14 to +16
if (props.isDrop !== "None") {
if (props.isDrop === "Menu1") {
if (props.menuNum === 1) {
Copy link

Choose a reason for hiding this comment

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

중첩된 조건문을 제거해보세요.

조건 상황을 같이 체크할 수 있으면 그렇게 하고요
if( a && b)

Comment on lines +6 to +7
ele.target.style.color = "#333";
ele.target.style.textDecoration = "underline";
Copy link

Choose a reason for hiding this comment

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

style값 지정은 어떤 상태에 의해서 styled componet에서 지정해주는 것이 더 좋죠.

);
} else return null;
} else {
if (props.menuNum === 3) {
Copy link

Choose a reason for hiding this comment

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

2,3 이런 하드코딩은 보다는 의미를 담으면 더 좋을 듯.

<Li>
<Span
_headMenu
key={3}
Copy link

Choose a reason for hiding this comment

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

이 key 값은 보통 서버에서 주는 고유한 id를 사용하게 될거에요.

import Span from "../../atoms/Span";

const HoverCard = ({ children, ...props }) => {
if (props.isHover) {
Copy link

Choose a reason for hiding this comment

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

props.isHover && (...)

이런식으로 && 연산자도 활용해보세요.

null반환은 따로 안해도 될거고요.


const thumbnails = _thumb_images;

const onClick = ({ target }) => {
Copy link

Choose a reason for hiding this comment

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

({target:{src}})

display: flex;
`;

const InfoGeneral = ({ children, ...props }) => {
Copy link

Choose a reason for hiding this comment

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

(props) => {}

본문코드보면, 이렇게 해도 동일할거 같은데요?

props.data.slice(-SLIDES).concat(props.data.slice(0, -SLIDES))
);
}
directionRef.current.style.transform = "translate(0)";
Copy link

Choose a reason for hiding this comment

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

DOM에 직접 접근하지말고, 어떤 상태를 변경시키고, 그 상태를 기반으로 node의 스타일이 결정되는 방식을 생각해보세요.
ref사용말고요.

@crongro crongro merged commit 0fd25d6 into codesquad-members-2021:team15 May 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review-FE FE 리뷰
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants