Skip to content

파이널 코딩 테스트 1번 문제 - 정글 시네마 - 병민#19

Open
merrybmc wants to merge 2 commits intomainfrom
1-byeongMin
Open

파이널 코딩 테스트 1번 문제 - 정글 시네마 - 병민#19
merrybmc wants to merge 2 commits intomainfrom
1-byeongMin

Conversation

@merrybmc
Copy link
Copy Markdown
Collaborator

💡 프로젝트 실행 방법

git clone https://github.com/FRONTENDSCHOOL7/CodeReviewStudy.git
npm install
npm start

📌 배포 링크

http://junglecinema.s3-website-us-east-1.amazonaws.com/

📜 폴더 구조

📄 src
├── components
│   ├── footer
│   │   ├── Footer.jsx
│   │   └── Footer.scss
│   ├── main
│   │   ├── main
│   │   │   ├── Main.jsx
│   │   │   └── Main.scss
│   │   ├── article
│   │   │   ├── Article.jsx
│   │   │   └── Article.scss
├── pages
│   ├── Mainpage.jsx
│   └── Mainpage.scss
├── App.js
├── index.js
└── index.scss

🚀 추가 기능 구현

  • 상단 nav바 리스트 클릭 시 border값 변경
  • 예매하기 버튼 클릭 시 링크 이동
  • 예매하기 버튼 hover시 background color 변경
  • footer SNS 링크 클릭 시 링크 이동
  • footer SNS 링크 마우스 hover시 scale 효과 적용

🙋‍♀️ 중점적으로 리뷰 받고 싶은 내용

  1. 반응형 페이지를 올바르게 구현했는지 궁금합니다.
  2. 시맨틱 태그를 고려하여 작성하였는데 더 나은 부분이나 고쳐야할 부분이 있는지 피드백받고 싶습니다.

@merrybmc merrybmc self-assigned this Jul 31, 2023
@merrybmc merrybmc changed the title [#1] 정글시네마 과제 구현 파이널 코딩 테스트 1번 문제 - 정글 시네마 - 병민 Jul 31, 2023
@gyeongbaek
Copy link
Copy Markdown

  1. 현재 sass가 package.json에 없어 오류가 발생합니다.
  2. 영화목록 ()의 레이아웃이 깨집니다. -> 반응형 코드를 수정해서 최대 너비를 지정해주시는게 좋을 것 같습니다
  3. nav 바 아이템에 h2가 들어가있는데 nav바 보다는 cardArticle에 들어가는게 좋을 것 같아요.
  4. .card에서 버튼 컨테이너의 레이아웃이 깨집니다. -> border-radius, border 등 css 수정이 필요합니다.
  5. card 내부 정보가 ol로 되어있는데, 저라면 ol보다는 dl이나 ul을 사용할 것 같아요. 저 내용들이 순서가 필요한 요소들은 아니라, 좀 더 시맨틱한 코드를 고민해보세요!
  6. footer의 address가 지금 텍스트로 들어가있는데요, 디자이너님은 텍스트로 주셨더라도, 시맨틱하게 코드를 작성해주셔야 합니다.

리드미에 현재 실행 방법을 작성해주셨는데, 해당 명령어를 실행하면 오류가 발생합니다. 실제 면접관이 하나하나 실행해보진 않겠지만 코드만 봐도 제대로 실행되지 않는다는 것을 알 수 있기 때문에 수정해주세요! 코드에 관한 피드백은 코드에 추가해두겠습니다!
코드 작성하시느라 수고 많으셨습니다 :)

@merrybmc
Copy link
Copy Markdown
Collaborator Author

  1. 현재 sass가 package.json에 없어 오류가 발생합니다.
  2. 영화목록 ()의 레이아웃이 깨집니다. -> 반응형 코드를 수정해서 최대 너비를 지정해주시는게 좋을 것 같습니다
  3. nav 바 아이템에 h2가 들어가있는데 nav바 보다는 cardArticle에 들어가는게 좋을 것 같아요.
  4. .card에서 버튼 컨테이너의 레이아웃이 깨집니다. -> border-radius, border 등 css 수정이 필요합니다.
  5. card 내부 정보가 ol로 되어있는데, 저라면 ol보다는 dl이나 ul을 사용할 것 같아요. 저 내용들이 순서가 필요한 요소들은 아니라, 좀 더 시맨틱한 코드를 고민해보세요!
  6. footer의 address가 지금 텍스트로 들어가있는데요, 디자이너님은 텍스트로 주셨더라도, 시맨틱하게 코드를 작성해주셔야 합니다.

리드미에 현재 실행 방법을 작성해주셨는데, 해당 명령어를 실행하면 오류가 발생합니다. 실제 면접관이 하나하나 실행해보진 않겠지만 코드만 봐도 제대로 실행되지 않는다는 것을 알 수 있기 때문에 수정해주세요! 코드에 관한 피드백은 코드에 추가해두겠습니다! 코드 작성하시느라 수고 많으셨습니다 :)

감사합니다! 피드백 내용을 토대로 코드를 수정해보겠습니다. 😀

import Article from '../Article/Article';

export default function Main() {
const [navState, setNavState] = useState({ current: 'true', due: 'false', boxOffice: 'false' });
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

navState를 객체로 이용하고 계시는데, current / due / boxOffice 값을 주는게 더 간단할 것 같아요.

onClick 이벤트에서 그냥 값으로 변경해도 되지만, button의 name 값을 지정하고, 이벤트 객체를 이용해서 값을 변경하시는 것도 좋습니다. <- 추가적인 사항이니 어려우시면 값으로 지정하셔도 됩니다.

<li>
<h2>
<button
className={`navTitle ${navState.current ? 'true' : 'false'}`}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

navState 객체의 값이 true / false에 따라 활성화될 클래스를 설정하고 계시는데 true / false 보다는 명확한 클래스명을 사용해주시면 좋을 것 같습니다. false에 따로 스타일이 없다면 ? 'ture': '' 이렇게 작성해주셔도 됩니다.


return (
<article className='cardArticle'>
{movieData?.map((data) => {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

데이터가 정적으로 들어오기 때문에 데이터가 없는 경우가 고려되어 있지 않는데, 통신으로 받아오는 프로젝트는 값이 비어있을 경우가 있기 때문에 레이아웃이 깨지지 않게 설정해주시거나 데이터가 없다는 것을 알 수 있도록 구현해주시면 더 좋을 것 같아요!

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Warning: Each child in a list should have a unique "key" prop.

Key는 React가 어떤 항목을 변경, 추가 또는 삭제할지 식별하는 것을 돕습니다. key는 엘리먼트에 안정적인 고유성을 부여하기 위해 배열 내부의 엘리먼트에 지정해야 합니다.

https://ko.legacy.reactjs.org/docs/lists-and-keys.html

return (
<section className='card'>
<div className='textGroup'>
<img className='poster' alt='poster' src={require(`${data.image}`)}></img>
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

img의 alt 속성은 h3 태그로 영화의 제목 정보를 주고 있기 때문에 비워두셔도 괜찮을 것 같습니다.

Copy link
Copy Markdown
Collaborator

@YennieJ YennieJ left a comment

Choose a reason for hiding this comment

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

rem을 사용하셨군요..! 고생하셨습니다👍

1.반응형

  • Main.jsx +30 (headline의 뜻은 제목,표제 이런 느낌이라서 이름을 변경하는게 좋을것 같아요 )
  • Footer.jsx 전체
    위 두개 width 변경을 고려해보시면 좋을꺼같습니다.
    화면이 작아지면 scroll이 생겨요 :)
    변경 하시면 mediaQuery도 width값으로 설정 가능할것 같습니다
  1. font 설정이 안되어있어요!
    font-style,line-height등 기본 설정인것은 작성하지 않아도 될거같습니다:)
  2. Article.jsx 평점 옆에 별 이미지가 빠졌어요 :)

Comment on lines +20 to +23
<header className='mainHeader'>
<h1 className='mainTitle'>정글 시네마 영화 목록</h1>
<img src={headLine} alt='headLine' />
</header>
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

header 안에 nav가 들어가도 될거같아요 :)


const handleNavState = (state) => {
setNavState({ current: state === 'current', due: state === 'due', boxOffice: state === 'boxOffice' });
console.log(navState);
Copy link
Copy Markdown
Collaborator

@YennieJ YennieJ Aug 15, 2023

Choose a reason for hiding this comment

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

사용하지 않는 console은 삭제하는게 좋을것 같습니다 :)


return (
<article className='cardArticle'>
{movieData?.map((data) => {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Warning: Each child in a list should have a unique "key" prop.

Key는 React가 어떤 항목을 변경, 추가 또는 삭제할지 식별하는 것을 돕습니다. key는 엘리먼트에 안정적인 고유성을 부여하기 위해 배열 내부의 엘리먼트에 지정해야 합니다.

https://ko.legacy.reactjs.org/docs/lists-and-keys.html

gap: 2rem 1.44rem;

justify-content: center;
padding: 2rem 0;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

화면이 작아질때 border끼리 겹쳐져서 양 옆에도 padding이 들어갔으면 좋겠어요!

<img className='poster' alt='poster' src={require(`${data.image}`)}></img>
<h3 className='title'>{data.title}</h3>
<ol className='info'>
<div className='subjectGroup'>
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

ol,ul 직계자식은 li만 사용한다고해요!

Comment on lines +52 to +54
.footerSVG:hover {
transform: scale(1.3);
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

👍👍
trasition도 사용해보면 더 멋있을거 같아요

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants