-
Notifications
You must be signed in to change notification settings - Fork 0
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
KDT0_HanEunJi 캐릭터 관리 서비스 #46
base: main
Are you sure you want to change the base?
Conversation
UI/UX 너무너무 귀여워용 ㅠㅜㅠㅜ 과제하느라 넘 수고하셨어요!! 갠적으로 너비 줄인 상세 페이지가 넘 이뿌네용~~ 이미지 파일 선택 시의 미리 보기랑 상세 페이지에서 뒤로 가기 버튼 있었으면 더 좋을 것 같단 생각이 들었습니다! |
init.js 되게 신기하네요! 디자인 깔끔하고, firebase를 유용하게 잘 사용하신것같습니다!! 그런데 삭제하시겠습니까 모달창이 조금 큰 것 같긴합니다 !! |
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.
은지님도 프로젝트 고생하셨습니다!
코드 깔끔해고 가독성이 좋아서 리뷰하기 편했습니다~
window.process = { | ||
env: { | ||
NODE_ENV: 'development', | ||
}, | ||
}; |
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.
이 방법으로도 env 파일 사용이 가능하군요..
webpack에서 env 관련 외부 모듈을 가져와서 사용하는 것만 가능한 줄 알았는데 아니었군요!
{ | ||
"liveServer.settings.port": 5501 | ||
} |
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.
오 liveServer port도 설정 가능하군요 꿀팁 줍줍 감사합니다.
.profile-container { | ||
display: grid; |
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.
프로필 디자인을 flex layout이 아닌 grid로 하신 이유가 궁금합니다!
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.
그리드 속성으로 grid-template-columns: 1fr 1.1fr;
리고 작성했었는데 프로필 내용 영역을 사진보다 조금만 더 크게 잡고 싶었어서 그렇게 작성했었습니다😅
@media (max-width: 740px) { | ||
.header-wrapper { | ||
width: 290px; | ||
height: 105px; | ||
} | ||
.header-title { | ||
font-size: 22px; | ||
} | ||
} | ||
|
||
@media (min-width: 1440px) { | ||
.header-wrapper { | ||
width: 860px; | ||
height: 212px; | ||
} | ||
.header-title { | ||
font-size: 43px; | ||
} | ||
} |
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.
저도 sass에서 mixin안에 너비가 범위가 지정된 media 쿼리를 선언해서 필요할 때마다 불러와 디자인하는 방식으로 반응형을 구현하였는데, 바보같이 너비 범위만 지정하고 미디어 쿼리 안에 폰트 지정을 안해줬네요,,,
때문에 요소 하나하나 에 mixin을 include로 불러와서 폰트 사이즈 지정해 줬는데 미디어 쿼리 이점을 전혀 못살렸네요 반성합니다 ㅠㅠㅋㅋㅋㅋ ('영수'했네요)
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.
ㅋ ㅋ ㅋ ㅋ ㅋ ㅋ ㅋ ㅋ ㅋ ㅋ 영수라면 그럴수있죠..ㅎㅎ
firebase.js
Outdated
export { getDocs, getDoc, addDoc, deleteDoc, doc, updateDoc } from 'https://www.gstatic.com/firebasejs/10.1.0/firebase-firestore.js'; | ||
export { ref, uploadBytesResumable, getDownloadURL, deleteObject } from 'https://www.gstatic.com/firebasejs/10.1.0/firebase-storage.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.
오호 firebase.js에서 한번에 필요한 모듈만 한번에 export 하는 방법도 괜찮네요!
js/character.js
Outdated
getDocs(colRef) | ||
.then((snapshot) => { | ||
snapshot.docs.forEach((doc) => { | ||
const $character = document.createElement('a'); |
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.
jQuery를 써본 적이 없어 잘 모르는데.. 혹시 변수명 앞에 $를 붙이는 이유가 궁금합니다!
jQuery를 사용하지 않아도 붙이는 경우가 있다고 하는데 그 이유가 궁금합니다!
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으로 접근하는 요소를 구별하는 용도로 사용하기 시작했었는데 어느샌가 무지성 남발중이더라구요,, 반성하구 갑니다,,,,
js/index.js
Outdated
window.addEventListener('scroll', () => { | ||
const $scrollY = window.scrollY; | ||
console.log($scrollY); | ||
$modalBackground.style.top = `${$scrollY}px`; | ||
}); |
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.
모달 고정 방법이 궁금했는데 scrollY를 top px로 고정하는 방법이군요!
display: fixed 방법은 적용 되지않아서 사용하셨는지 궁금합니다!
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.
음 매 스크롤마다 스크롤 위치를 기억하는 방법보다는 position: fixed 로 조금 더 쉽게 처리할 수 있었을 것 같습니다!
js/upload.js
Outdated
getDownloadURL(uploadTask.snapshot.ref).then((downloadURL) => { | ||
console.log('File available at', downloadURL); | ||
addDoc(colRef, { | ||
image: downloadURL, |
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.
처음에 storage에 있는 img 정보를 어떻게 firestore field와 연결할지 공식 문서에 없어서 고민했었는데 같은 방법이라 뭔가 안도? 되네요.
은지님은 어떻게 떠올리셨는지 궁금합니다! 이게 일반적인 방법일까요?
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.
저도 정확한 코드를 찾은 건 아니지만 구글링해보니 url을 사용하는 게 일반적인 방법같더라구요..! url은 파일을 업로드할 때 바로 얻을 수 있기도 하고 console.log('File available at', downloadURL);
를 보고 힌트를 얻었던 것 같습니다.
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.
은지님은 과제 하나하나에 정말 정성을 다해서 하시는 분 같습니다.
JS과제인데도 불구하고 반응형도 정말 잘 작동하고 디자인적인 요소들도 정말 많이 배웠습니다.
다음 토이프로젝트도 기대하고 있겠습니다!! 이번 피어리뷰 때도 많은 것을 배우고 갑니다!!
{ | ||
"liveServer.settings.port": 5501 | ||
} |
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.
저도 너무 신기해서 바로 해봤는데
잘 작동하더라고요!!
역시 재야의 고수 은지님이십니다...😎
assets/images/footer-image.png
Outdated
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.
웹페이지라면 정말 디자인 적인 요소가 필수라고 생각합니다.
이러한 png를 통한 footer 구분은 타고나신 센스인 것 같습니다.
이번에 테스트 겸 거북이 사진 올리는 거 많이 고민했습니다.... 너무 귀여운 캐릭터들 밖에 없어서 큰 고민하고 올렸습니다.
justify-content: space-evenly; | ||
} |
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.
justify-content의 이런 속성이 또 있었네요..!!
또 하나 배워갑니다.
css/components/profile.css
Outdated
.profile__image { | ||
width: 100%; | ||
height: 100%; | ||
padding: 1em; | ||
|
||
background-size: contain; | ||
background-repeat: no-repeat; | ||
background-position: center; | ||
} |
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강의가 끝나고 많이 부족한 것 같아 찾아봤더니
Backgound-image와 Image 태그를 쓰는 경험적인 차이점이 있다고 배웠습니다.
딱 정확한 예시로 작성하신 것 같아 또 하나 배우고 갑니다!!
.sketchy::before { | ||
content: ''; | ||
display: block; | ||
|
||
width: 100%; | ||
height: 100%; | ||
|
||
position: absolute; | ||
top: 50%; | ||
left: 50%; | ||
|
||
border: 2px solid #353535; | ||
border-radius: 1% 1% 2% 4% / 2% 6% 5% 4%; | ||
transform: translate3d(-50%, -50%, 0) scale(1.015) rotate(0.5deg); | ||
z-index: -1; | ||
} |
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.
이런 디테일을 보고 정말 감탄했습니다.
어떻게 기본 CSS만으로 이런 테두리를 만들고 디자인 요소를 만드셨는지....
border-radius를 이렇게 활용할 수 도 있다는 것을 배워갑니다!!
역시 재야의 고수 은지님...😎
import { initializeApp } from 'https://www.gstatic.com/firebasejs/10.1.0/firebase-app.js'; | ||
import { getFirestore, collection } from 'https://www.gstatic.com/firebasejs/10.1.0/firebase-firestore.js'; | ||
import { getStorage } from 'https://www.gstatic.com/firebasejs/10.1.0/firebase-storage.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.
firebase 모듈을 다운 받지 않고도 이렇게 사용할 수 있는 지 처음 알았습니다.
이것도 cdn을 통한 방법인지 궁금합니다!!
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.
CDN이 맞습니다! 다만 공식문서에서는 모듈 번들러와 함께 모듈식 API를 사용하는 것을 권장하지만 저는 이번 프로젝트에서 번들러를 사용하지 않을 예정이었기 때문에 CDN을 사용했습니다..!
js/index.js
Outdated
const $scrollY = window.scrollY; | ||
console.log($scrollY); |
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.
처음 개발자 도구로 보고 있었는데, jquery는 아예 모르지만 지금 보는 scrollY 값이 console에 찍히고 있었습니다!
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.
알려주셔서 감사합니다..!!
js/index.js
Outdated
window.addEventListener('scroll', () => { | ||
const $scrollY = window.scrollY; | ||
console.log($scrollY); | ||
$modalBackground.style.top = `${$scrollY}px`; | ||
}); |
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.
저도 모달창이 계속 움직여서 불편했는데 이렇게 해법을 얻고 갑니다!!
js/profile.js
Outdated
const $scrollY = window.scrollY; | ||
console.log($scrollY); | ||
$modalBackground.style.top = `${$scrollY}px`; |
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.
여기에도 scrollY값이 나오고 있습니다!
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.
.character { | ||
display: flex; | ||
flex-direction: column; | ||
align-items: center; | ||
|
||
width: 320px; | ||
height: 340px; | ||
|
||
margin: 40px; | ||
padding: 20px; | ||
|
||
background-color: #fff; | ||
transform: rotate(1deg); | ||
} |
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.
넵넵 언제나 좋은 피드백 너무 감사드립니다...🥹
border: none; | ||
background-color: transparent; | ||
cursor: pointer; |
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.
button에 대한 기본 reset 설정이죠? 이런 건 reset.css 와 같은 곳에 만들어서 일괄적용되게 하는 게 좋습니다.
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.
앗 다른 버튼들은 기본 스타일을 사용하고 있어서 해당 버튼에만 따로 적용한 속성이었습니다..! 다음에 기본 reset 설정을 적용할 때 멘토님 말씀 반영해보겠습니다. 피드백 감사합니다!!
main.css와 같은 파일에서 단순하게 해당 설정을 일괄 적용하는 것과, 기존 스타일을 초기화하는 설정들은 따로 모아 관리하는 것 중 후자가 더 권장되는 방법이라고 이해하면 될까요?
css/components/modal.css
Outdated
width: 100vw; | ||
height: 100vh; |
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.
right:0;
bottom: 0;
으로도 해결할 수 있는데, 어떤 차이인지 공부해보세요!
css/components/modal.css
Outdated
position: absolute; | ||
top: 50%; | ||
left: 50%; | ||
transform: translateX(-50%) translateY(-50%); |
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.
transform: translateX(-50%) translateY(-50%); | |
transform: translate(-50%, -50%); |
으로도 가능합니다!
css/components/profile.css
Outdated
background-size: contain; | ||
background-repeat: no-repeat; | ||
background-position: center; |
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.
background는 단축 속성으로 한 줄 표기를 쓰는 사람들이 많은데, 은지님은 이렇게 적으신 이유가 따로 있으신지 궁금합니다. 🤔
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.
따로 작성하는 것이 익숙하다 보니 계속 그렇게 작성해오고 있었습니다😭 위에 말씀해주신 transform도 포함해서 앞으로 더 단축 속성에 신경쓰며 코드 작성하도록 하겠습니다..!
js/index.js
Outdated
const $cancel = document.querySelector('.button--cancel'); | ||
|
||
window.addEventListener('scroll', () => { | ||
const $scrollY = window.scrollY; |
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으로 접근해야 하는 변수에만 $을 붙이고 나머지 변수들은 그냥 $ 없이 변수명을 지었다면 조금 더 가독성을 챙길 수 있지 않을까? 라는 생각을 해봤습니다.
js/index.js
Outdated
window.addEventListener('scroll', () => { | ||
const $scrollY = window.scrollY; | ||
console.log($scrollY); | ||
$modalBackground.style.top = `${$scrollY}px`; | ||
}); |
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.
음 매 스크롤마다 스크롤 위치를 기억하는 방법보다는 position: fixed 로 조금 더 쉽게 처리할 수 있었을 것 같습니다!
js/profile.js
Outdated
@@ -0,0 +1,115 @@ | |||
import { db, doc, getDoc, ref, deleteDoc, deleteObject, storage } from '../firebase.js'; | |||
|
|||
let queryString = new URLSearchParams(window.location.search); |
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 로 선언했으면 어땠을까요?
js/profile.js
Outdated
|
||
export const docRef = doc(db, 'characters', queryString.get('id')); | ||
export const docSnap = await getDoc(docRef); | ||
console.log(docSnap.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.
배포까지 진행할 때는, 이런 불필요한 로그는 없는지 확인해보는 습관을 들여보시길 바랍니다!
js/profile.js
Outdated
$profileImage.style.backgroundImage = `url(${docSnap.data().image})`; | ||
$infoName.textContent = `이름 : ${docSnap.data().name}`; | ||
$infoAppearance.textContent = `첫 등장 : ${docSnap.data().appearance}`; | ||
$infoInterests.textContent = `좋아하는 것 : ${docSnap.data().interests}`; |
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.
docSnap.data()
이 계속 반복되어서 호출되고 있죠..?! 한 번만 호출해서 변수에 담았다면 어땠을까요?!
캐릭터 관리 서비스
✨ 프로젝트 소개
캐릭터를 등록하고 정보를 관리할 수 있는 서비스입니다.
✨ 필수 요구사항
✨ 기술 스택
✨ 주요 기능
✨ 유저 플로우