-
Notifications
You must be signed in to change notification settings - Fork 8
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
[FE] 생성된 약속 링크를 공유하는 페이지 UI, 클립보드 복사 기능 구현 #77
Conversation
Test Results0 tests 0 ✅ 0s ⏱️ Results for commit 2f1088a. ♻️ This comment has been updated with latest results. |
리뷰하기전에 일단 너무 귀엽다 |
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: center; | ||
|
||
width: 100%; | ||
height: calc(100vh - 6rem); |
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.
[P3]
사소하지만, 헤더 외 나머지 높이를 적용하고 싶을 때 부모 태그에 flex 적용 후 해당 컴포넌트에 flex: 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.
오...진짜 개꿀팁
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: 1
사용하니까 훨씬 편하네요!
사용 시도하고 잘 적용되는 것도 확인했습니다.
다만, 변경할 때 상위 GlobalLayout
컴포넌트의 s_globalContainer
에서 display: flex;
를 적용해 줘야해서
현재 낙타와 해리가 작업 중인 스타일에 영향을 줄 것 같습니다🥹😭
추후 한 번에 수정하도록 하겠습니다!
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.
제가 제안한 네이밍이 반영되었네요!🥳🥳🥳
감사합니다 🐫
@@ -0,0 +1,8 @@ | |||
export const copyToClipboard = async (text: string) => { |
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.
[Q]
여기 함수 이름을 어떻게 통일 시키면 좋을까요? 이것도 얘기를 나누어보면 좋겠네요!
export const a = () => {}
export function 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.
2차 스프린트 끝나고 얘기나눠봐야할 것 같아요
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.
빙봉 고생해쓰요~
함수 컨벤션, 약속 생성 페이지 타이틀 네이밍에 관해서는 나중에 한 번 얘기 나눠봐야한다는 것을 인지해주면 좋을 것 같아유~
수정할 부분이 보이지 않아 approve 합니다!
{/* TODO: '${약속 명} 약속을 만들었어요 :)'로 변경 논의 후 적용 (@Yoonkyoungme) */} | ||
<div css={s_description}>약속을 만들었어요 :)</div> |
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.
오~ 약속명 붙이는 것 좋아요
8afcad8
to
2f1088a
Compare
관련 이슈
작업 내용
copyToClipboard
를 추가 (utils/clipboard.ts)특이 사항
→ 복사 되었다는 것을 명시적으로 표현하기 위해 일시적으로 alert를 띄우게 해두었습니다.
→ 복사할 텍스트는 임의로 상수 데이터로 설정하였습니다. 추후
prop data
또는useLocation
에서 보내주는 값으로 교체해야 할 것 같아요.리뷰 요구사항 (선택)