Skip to content

Chore/project jy#29

Open
Jeongyun-Jang wants to merge 14 commits intodevelopfrom
chore/project_jy
Open

Chore/project jy#29
Jeongyun-Jang wants to merge 14 commits intodevelopfrom
chore/project_jy

Conversation

@Jeongyun-Jang
Copy link
Copy Markdown

@Jeongyun-Jang Jeongyun-Jang commented Jun 13, 2024

Reference 📚


Motivation 🤔

  • 상세 페이지 작업
  • 상단 사진 영역, 리뷰 영역 swiper 반영
  • RecommendTravelCard (주변 여행지 추천 카드) 추가
  • ReviewCard (리뷰 카드) 추가

Key Chains 🔑


To Reviewers 🙏

  • WithTitleWrapper 타이틀 옆에 버튼이 있을 것을 고려해서 WithTitleWrapper 컴포넌트 수정하려 합니다.!

코드 의견 주시면 감사하겠습니다 :)

package.json Outdated
Comment on lines +25 to +28
"next": "14.1.0",
"qs": "^6.11.2",
"react": "^18.3.1",
"react-dom": "^18.3.1",
"react": "^18",
"react-dom": "^18",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

image

next 버전을 내리신 이유가 있을까요? 버전 올리라는 경고가 나오네요

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

swiper 설치하신거 아닌가요?
city/[id] page들어가니 에러가 나서 보니 package.json에 없네요??

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

conflict 해결 과정에서 packge.json이 수정된것 같습니다.

Comment on lines +11 to +13
const PACKAGE_VERSION = '2.2.13'
const INTEGRITY_CHECKSUM = '26357c79639bfa20d64c0efca2a87423'
const IS_MOCKED_RESPONSE = Symbol('isMockedResponse')
Copy link
Copy Markdown
Contributor

@dev-yun dev-yun Jun 17, 2024

Choose a reason for hiding this comment

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

.prettierrc.json에 "singleQuote": false 옵션을 줬는데도 정윤님 auto save할때는 자동으로 single quete로 저장되나보군요

이유가 뭘까요? 하나로 맞추지 않으면 pr마다 변경사항이 발생할 것 같습니다
(따로 설정파일 있으시면 공유해주셔도 좋을 것 같아요 pr에는 안보이네요)

Comment on lines +2 to +14
import React from "react";
import Image from "next/image";
import { Pagination } from "swiper/modules";
import { Swiper, SwiperSlide } from "swiper/react";
import LocationIcon from "@/components/icons/LocationIcon";
import { Tag } from "@/components/ui/Tag";
import Link from "next/link";
import WithTitleWrapper from "@/components/common/wrapper/WithTitleWrapper";
import RecommendTravelCard from "../../components/RecommendTravelCard/RecommendTravelCard";
import ReviewCard from "../../components/Review/ReviewCard";
import 'swiper/css';
import 'swiper/css/navigation';
import 'swiper/css/pagination';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lint에 sort-imports rule을 걸어놨는데 이게 통과가 됐군요 다시 봐야겠네요

Auto Import extension 설치해주시면 저희 사이에서 import 변경이 없을 것 같습니다..!

return (
<main>
<MobileHomeHeader />
<div className="flex p-4" style={{flexWrap:"wrap"}}>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

tailwind와 inner style이 혼합되어 쓰이네요

const placeCityType: PlaceCityType = {
"place-theme": "THEME",
"place-trending": "TRAVEL_PLACE",
"city-trending": "CITY",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

PlaceCityType에서 value가 "THEME", "TRAVEL_PLACE", "CITY"로 고정이라면 string이 아니라 ENUM등을 사용해서 타입을 더 좁힐 수 있을 것 같습니다 :)

Comment on lines +17 to +21
<div className="absolute block border rounded-3xl bg-gray-200 opacity-40 px-2">
<span>props city</span>
</div>
}
<img src={img} alt="place_image" className="block border border-grey-300 rounded-2xl" style={{height: "155px"}}/>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

next Image를 사용하는게 더 좋아보입니다~!

width, height에 164px에 맞는 tailwind 사이즈가 없어서 사용하신 것 같은데 tailwind.config에 추가하셔도 됩니다.
(이건 개인적인 의견인데 디자인에서는 화면 크기가 375px일때 row-gap: 15px, col-gap:24px로 하고 image는 나머지 공간을 꽉채우는 방식이 더 괜찮지 않을까요? 화면이 줄어들어도 문제고, 화면이 늘어나도 이미지 사이 공간만 커질 것 같아서요)


</div>
<MobileNavigation />
</main>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(home)/(list)/place-theme/page, (home)/(list)/place-trending/page, (home)/(list)/city-trending/page page에서 모두

 const pathname = usePathname();
 const key = pathname.split('/')[1] as keyof PlaceCityType;
 //...
 <PlaceList placeCityType={placeCityType[key] as "THEME" | "TRAVEL_PLACE" | "CITY"} />

위 코드를 사용하셨는데 각각의 페이지에서는 무조건 "THEME" | "TRAVEL_PLACE" | "CITY" 중 하나의 값을 갖으니까 <PlaceList placeCityType={"THEME"} />등으로 간단하게 해결할 수 있지 않을까요?

@dev-yun
Copy link
Copy Markdown
Contributor

dev-yun commented Jun 17, 2024

혹시 vscode setting.json에 아래 설정 추가 된 상태신가요?

"editor.codeActionsOnSave": {
    "source.fixAll.eslint": "explicit"
  }

auto save할때 lint에 맞게 수정해주는 옵션입니다~!

@@ -119,23 +118,11 @@ return <>
</div>
<Link href={"/place-trending"} className="text-[#b8b8b8] text-xs">더보기</Link>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

text-[#b8b8b8] => text-gray-300 😄

<div className="flex items-center">
<Card.StarButton />
<div className="w-1 h-50 px-2" />
<p className="text-[12px]">{'위치 보기'}</p>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

text-[12px] => text-xs 👍

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@dev-yun 다른 부분도 tailwind.config.ts 맞추어 수정하겠습니다 :)

Comment on lines +20 to +21
{useSideButton && <Link className="text-[12px] ml-2.5" href="/">{sideButtonTitle ?? '보기'}</Link>}
</div>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

image

WithTitleWrapper에서 useSideButton이 true일때 가 렌더링된다고 강제해도 될까요?

사진을 보면 작성인 경우는 Link보다 기능을 넣어야할 button일 수 있고, sidebarButtonTitle은 string이 아니라 React.Node가 될 수 있으며 href="/"도 특정 props로 수정되어야할 수 있는 등등

여러 가정이 떠올라서 외부에서 component를 주입받는 방식은 어떨까요?

Copy link
Copy Markdown
Author

@Jeongyun-Jang Jeongyun-Jang Jun 17, 2024

Choose a reason for hiding this comment

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

@dev-yun 넵 재사용을 고려하여 개선하는 편이 좋을 것 같습니다

props로 받아서 여러 가정을 처리할 생각이었는데,

제안하신 방법으로 좀 더 고민해보겠습니다 🙂

@@ -24,8 +24,8 @@
"lucide-react": "^0.344.0",
"next": "14.1.0",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

next version과 swipper library가 여전히 사라진 것 같습니다 ㅎㅎ

로컬에서 실행되시나요?? npm install 한번 더 하시면 안될 것 같은데

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.

2 participants