Feature/list view - Githubissues

답장 감사드립니다! 중간고사 끝나는대로 바로 수정 들어가겠습니다!!

2020년 4월 22일 (수) 오후 3:04, An GwangHo notifications@github.com님이 작성:

@AnGwangHo requested changes on this pull request.

작업하시느라 고생이 많으셨습니다. 코드 수정의 경우 log제거만 하시면 merge가능한 정도입니다.

-

현재 여러 컴포넌트나 환경적인 부분에 대하여 개발하셨는데 이에 대한 .test.tsx가 없습니다. 개발 방법론의 경우 TDD를 사용하기로 동의하였으므로 해당 부분에대해서는 작성해주셔야합니다. 또한, 해당 test case가 전부 통과되어야 develop브랜치에 merge가능합니다.

현재 ListView라는 브랜치에 환경설정(폰트, 라우터), 페이지 여러부분이 작업이 되어있는데 해당 부분에 대해서는 브랜치를 전부 분리하여 PR를 하는 것이 리뷰나 히스토리 관리 시 좋을 것 같습니다. 이 부분에 대해서 저도 미리 고지를 못하였기에 반성하고 있습니다.

추후 작업 시 환경설정, 페이지, 컴포넌트 등 1개의 브랜치에는 1개의 기능이 들어가는 것이 좋을 것 같습니다. 참고 블로그 https://brunch.co.kr/@anonymdevoo/9

In package.json https://github.com/YAPP-16th/Team_Web_2_Client/pull/74#discussion_r412684023 :

@@ -16,8 +16,10 @@ "@types/styled-components": "^5.1.0", "axios": "^0.19.2", "node-sass": "^4.13.1",

"query-string": "^6.12.1",

해시 라우팅을 위해서 추가한걸로 이해했습니다.

해당 부분에 대하여 PR에 라이브러리 언급과 사용한 이유가 기술되었으면 나중에 참고 시 좋을 것 같습니다.

In src/App.test.tsx https://github.com/YAPP-16th/Team_Web_2_Client/pull/74#discussion_r412685087 :

it("first test", () => {

const utils = render();

utils.getByText("리스트뷰 +");

const utils = render();

App.tsx에 대한 test case를 작성하는게 맞지 않을까요?

Provider의 경우 index.tsx에서 설정하므로 index.test.tsx에서 해주는게 좋을 것 같습니다.

개인적인 의견으로는 App.test.tsx에서는 URI에 맞는 컴포넌트가 로딩되어 있는지만 테스트 해주면 될 것같습니다.

In src/components/Icon/Icon.tsx https://github.com/YAPP-16th/Team_Web_2_Client/pull/74#discussion_r412687238 :

@@ -0,0 +1,53 @@ +import React from "react"; +import * as icons from "../../assets/svgs/icons"; +import styled from "styled-components"; + +export type IconType = keyof typeof icons; +export const iconTypes: IconType[] = Object.keys(icons) as any[]; // 스토리에서 불러오기 위함 + +type IconProps = {

주석의 경우 안에다가 /

/형식으로 작성해주셨는데 IconProps 상단에 아래와 같이 다는 것은 어떨까요? /

icon : 아이콘 색상

... **/

In src/pages/HeaderPage/HeaderPage.tsx https://github.com/YAPP-16th/Team_Web_2_Client/pull/74#discussion_r412689536 :

+import ZoneDetailHeaderContainer from '../../containers/HeaderContainer/ZoneDetailHeaderContainer'; +import ZoneSearchResultHeaderContainer from '../../containers/HeaderContainer/ZoneSearchResultHeaderContainer'; + +type ParamsType = {

id: string;

feature: string; +}

+const HeaderPage = ({ match, location, history}: RouteComponentProps) => {

const query = queryString.parse(location.search);

let HeaderContainer;

// 이것도 zone을 뒤에 붙일 수 있는거면 좋겠는데

console.log(location.hash);

console.log제거 부탁드립니다.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/YAPP-16th/Team_Web_2_Client/pull/74#pullrequestreview-397856913, or unsubscribe https://github.com/notifications/unsubscribe-auth/AH3UNDQAYZEKUAFB27NP4X3RN2CHBANCNFSM4MMMQJ6Q .

YAPP-16th / Team_Web_2_Client

Feature/list view #74

해당 부분에 대하여 PR에 라이브러리 언급과 사용한 이유가 기술되었으면 나중에 참고 시 좋을 것 같습니다.