-
Notifications
You must be signed in to change notification settings - Fork 5
프로그래머스 고양이 사진 검색 사이트 #25
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨습니다 기봉형! 계속 반복풀이하시죠!
mouseover구현하셨다고 적혀있는데 관련 코드가 어딨을까요??
.ImageInfo { | ||
position: fixed; | ||
left: 0; | ||
top: 0; | ||
width: 100vw; | ||
height: 100vh; | ||
background-color: rgba(0, 0, 0, 0.5); | ||
-moz-transition: opacity 0.7s linear; | ||
-o-transition: opacity 0.7s linear; | ||
-webkit-transition: opacity 0.7s linear; | ||
transition: opacity 0.7s linear; | ||
} | ||
|
||
.fadein { | ||
-webkit-animation: fadein 0.7s linear forwards; | ||
animation: fadein 0.7s linear forwards; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
animation이랑 transition 적용하셨군요 전 엄두도 못냈는데 ㅜ 👍
getInfo: async(id) => { | ||
try { | ||
console.log(id, "id") | ||
const result = await fetch(`${API_ENDPOINT}/api/cats/${id}`) | ||
console.log("result id info", result); | ||
return result.json(); | ||
} catch (e) { | ||
console.log(e); | ||
alert(e); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
에러발생시 alert만 띄워주고 return을 하지않는데 이렇게하면 받는쪽에서 데이터 없다는 에러가 또 발생하지 않나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오 그렇군요! 몰랐네요. 사실 에러 처리 할까 하다가 다른거 구현 하로 간거라 확인을 못했네요 ㅠ
const btn = document.createElement("BUTTON"); | ||
btn.innerHTML = "CLICK ME"; | ||
$target.appendChild(btn); | ||
|
||
btn.addEventListener("click", e => { | ||
const empty = document.getElementById("empty"); | ||
if(empty) { | ||
empty.remove(); | ||
} | ||
this.addLoadText(); | ||
|
||
onRandom(); | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
랜덤버튼이 Input의 자식이 아니라 target의 자식이니 따로 분리하는게 좋아보입니다!
node.setAttribute("id", "empty"); | ||
const textnode = document.createTextNode("empty"); | ||
node.appendChild(textnode); | ||
const app = document.getElementById("App").appendChild(node); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$target에서 이미 App객체를 갖고있는데 getElemenyById를 한번 더 하신이유가 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
엇 못봤네요 ㅋㅋㅋㅋ
closeModal(event) { | ||
event.stopPropagation(); | ||
|
||
event.target.parentNode.parentNode.parentNode.classList.add("fader"); | ||
event.target.parentNode.parentNode.parentNode.classList.add("fadedOut"); | ||
|
||
setTimeout(() => { | ||
event.target.parentNode.parentNode.parentNode.style.display = 'none' | ||
event.target.parentNode.parentNode.parentNode.classList.remove("fadedOut"); | ||
}, (500)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
부모를 계속 타고가는 이유를 알 수 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dom 찾는 query 등을 다시 사용 하기 싫어서 썼습니다 ㅋㅋ
mouseover img tag의 title 속성 넣어주면 바로 되요! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
전체적으로 분리가 잘 안되어있는 코드 같았습니다. 어떤건 constructor 안에서 수행하고 어떤건 밖에서 수행하고 딱히 기준점이 없었던 것 같고, 관심사의 분리가 안되어있고 그냥 파일 안에 로직을 때려박은 느낌이 강했습니다. 다음에는 일정한 기준을 두고 코드를 짜시면 더 읽는 데 예측가능할 것 같습니다!
수고하셨습니다~
fetchCats: async (keyword) => { | ||
try { | ||
const result = await fetch(`${API_ENDPOINT}/api/cats/search?q=${keyword}`); | ||
return result.json(); | ||
} catch (e) { | ||
console.log(e); | ||
alert(e); | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제가 알기로 요렇게하면 500 에러 안잡아지는 걸로 알고 있는데 잡히는 거 확인해보셨나용?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이번에 고쳐야 겠네요!
try { | ||
console.log(id, "id") | ||
const result = await fetch(`${API_ENDPOINT}/api/cats/${id}`) | ||
console.log("result id info", result); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
불필요한 콘솔은 제출 전에 지우시는 게 좋을 것 같습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙆♂️
const node = document.createElement("div"); | ||
node.setAttribute("id", "empty"); | ||
const textnode = document.createTextNode("empty"); | ||
node.appendChild(textnode); | ||
const app = document.getElementById("App").appendChild(node); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- app이 사실상 this.$target이라 굳이 다시 찾아서 appendChild 안해도 될 것 같습니다!
- 그리고 TextNode를 하나 만들어서 붙여주는 방법도 괜찮지만 innerHTML이나 innerText 사용해서 간결하게 줄일 수 있을 것 같아요.
- 그리고 appendChild는 이제 레거시라고 합니다. append에 대해서 공부해보시면 좋을 것 같습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오오오 그렇군요!
closea(event) { | ||
event.stopPropagation(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
closea... 무슨뜻인가요 ㅋㅋㅋㅋㅋ 예아 클로지아~ 예아~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ㅋㅋㅋㅋ
addEvent() { | ||
const close = this.$imageInfo.getElementsByClassName('close')[0]; | ||
const ImageInfo = document.getElementsByClassName('ImageInfo')[0]; | ||
const contentWrapper = document.getElementsByClassName('content-wrapper')[0]; | ||
|
||
close.addEventListener("click", this.closeModal); | ||
ImageInfo.addEventListener("click", this.close); | ||
contentWrapper.addEventListener("click", this.closea); | ||
|
||
setTimeout(() => { | ||
this.$imageInfo.classList.remove("fadein"); | ||
this.$imageInfo.classList.remove("fader"); | ||
}, (500)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- getElementBy~ 메서드들은 레거시라고 하더라구요. querySelector로 바꿔보시는 건 어떨까요?
- 지금 setState에서 this.addEvent를 계속 호출해주고 있는데, 이렇게되면 이미지 상세를 볼 때마다 50번째줄부터 52번째 줄까지 이벤트가 3개씩 걸릴 것 같아요. 따로 이벤트를 제거해주고 있지 않으니까 상세를 3번 보면 총 9개의 이벤트가 걸리겠죠. 이렇게 되면 메모리 누수가 발생할 것 같아서, 이 부분은 어떻게 개선할 지 조금 고민해보셔야 할 것 같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵
$imageInfo.className = "ImageInfo"; | ||
this.$imageInfo = $imageInfo; | ||
$target.appendChild($imageInfo); | ||
const overlay = document.createElement("section"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요놈은 사용하는 곳이 어딘지 못찾겠네요! 불필요한 코드들은 정리해주세요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵
return result.json(); | ||
} catch (e) { | ||
console.log(e); | ||
alert(e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alert로 그냥 e 때려버리시면 ㅋㅋㅋㅋ alert 창에 [object Object] 나옵니다 ㅋㅋㅋㅋ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ㅋㅋㅋㅋㅋㅋ
api.fetchCats(keyword).then(({ data }) => { | ||
this.renderOutputs(data); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
문제 요구사항에 Promise 패턴을 async/await으로 바꾸라고 되어있었어서, api.js는 잘 바꿔주셨으니까 이것도 구냥 바꿔주시면 좋을 것 같습니당!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌
event.target.parentNode.parentNode.parentNode.classList.add("fader"); | ||
event.target.parentNode.parentNode.parentNode.classList.add("fadedOut"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parentNode.parentNode.parentNode.... 보다 더 좋은 방법은 없을까욥?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아니면 적어도 19번째줄부터 24번째줄까지 중복되니까 변수하나에 담아서 처리하는 것도 좋을 것 같아요. 저번에 프로그래머스에서 리뷰받아보니까 꼼꼼하게 보더라구요.
const grandParent...? = event.target.parentNode.parentNode.parentNode;
grandParent.classList.add("fader");
grandParent.classList.add("fadedOut");
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오호 그렇군요!
}); | ||
|
||
$searchInput.addEventListener("keydown", e => { | ||
if (e.keyCode === 13) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
매직넘버 빼주는 거 좋아하더라구요. 빼주시면 좋을 것 같습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵!
오오 부스트 캠프에서 못 받아 봤던 리뷰들을 여기서 받아 보네요 ㅋㅋ 두분 다 갑사합니다! |
HTML, CSS 관련
현재 HTML 코드가 전체적으로 <div> 로만 이루어져 있습니다. 이 마크업을 시맨틱한 방법으로 변경해야 합니다.
유저가 사용하는 디바이스의 가로 길이에 따라 검색결과의 row 당 column 갯수를 적절히 변경해주어야 합니다.
다크 모드(Dark mode)를 지원하도록 CSS를 수정해야 합니다.
이미지 상세 보기 모달 관련
디바이스 가로 길이가 768px 이하인 경우, 모달의 가로 길이를 디바이스 가로 길이만큼 늘려야 합니다.
필수
이미지를 검색한 후 결과로 주어진 이미지를 클릭하면 모달이 뜨는데, 모달 영역 밖을 누르거나 / 키보드의 ESC 키를 누르거나 / 모달 우측의 닫기(x) 버튼을 누르면 닫히도록 수정해야 합니다.모달에서 고양이의 성격, 태생 정보를 렌더링합니다. 해당 정보는 /cats/:id 를 통해 불러와야 합니다.
추가
모달 열고 닫기에 fade in/out을 적용해 주세요.검색 페이지 관련
페이지 진입 시 포커스가 input 에 가도록 처리하고, 키워드를 입력한 상태에서 input 을 클릭할 시에는 기존에 입력되어 있던 키워드가 삭제되도록 만들어야 합니다.
필수
데이터를 불러오는 중일 때, 현재 데이터를 불러오는 중임을 유저에게 알리는 UI를 추가해야 합니다.필수
검색 결과가 없는 경우, 유저가 불편함을 느끼지 않도록 UI적인 적절한 처리가 필요합니다.최근 검색한 키워드를 SearchInput 아래에 표시되도록 만들고, 해당 영역에 표시된 특정 키워드를 누르면 그 키워드로 검색이 일어나도록 만듭니다. 단, 가장 최근에 검색한 5개의 키워드만 노출되도록 합니다.
페이지를 새로고침해도 마지막 검색 결과 화면이 유지되도록 처리합니다.
필수
SearchInput 옆에 버튼을 하나 배치하고, 이 버튼을 클릭할 시 /api/cats/random50 을 호출하여 화면에 뿌리는 기능을 추가합니다. 버튼의 이름은 마음대로 정합니다.lazy load 개념을 이용하여, 이미지가 화면에 보여야 할 시점에 load 되도록 처리해야 합니다.
추가
검색 결과 각 아이템에 마우스 오버시 고양이 이름을 노출합니다.스크롤 페이징 구현
검색 결과 화면에서 유저가 브라우저 스크롤 바를 끝까지 이동시켰을 경우, 그 다음 페이지를 로딩하도록 만들어야 합니다.
랜덤 고양이 배너 섹션 추가
/api/cats/random50
api를 요청하여 받는 결과를 별도의 섹션에 노출합니다.코드 구조 관련
필수
API 의 status code 에 따라 에러 메시지를 분리하여 작성해야 합니다.https://programmers.co.kr/skill_check_assignments/4