Skip to content
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

온보딩 페이지 UI #7

Merged
merged 14 commits into from
Jul 10, 2024
Merged

온보딩 페이지 UI #7

merged 14 commits into from
Jul 10, 2024

Conversation

moong23
Copy link
Contributor

@moong23 moong23 commented Jul 8, 2024

Why need this PR❓

온보딩 페이지 구현
https://jdustar1222.atlassian.net/jira/software/projects/SCRUM/boards/1/backlog?selectedIssue=SCRUM-56

Changes ✌️

아직 백엔드 API가 완료되진 않아서 로직을 제외한 UI입니다.

  • 로그인 페이지
    /onboard
image -> `Carousel`에서 에러가 발생해서 일단은 Image로 올려두었습니다 ㅠㅠㅠ
  • 관심사 페이지
    /onboard/interest
image
  • EmailList 페이지
    /onboard/emailList
image

+) 시간 괜찮으시면 해당 PR merge 후 배포 한번 해주시면 QA 바로바로 진행할 수 있을 것 같아요!

@moong23 moong23 self-assigned this Jul 8, 2024
@moong23 moong23 marked this pull request as ready for review July 9, 2024 01:15
@moong23 moong23 requested a review from dladncks1217 as a code owner July 9, 2024 01:15
@moong23
Copy link
Contributor Author

moong23 commented Jul 9, 2024

일단 Interaction이 필요해서 'use client' 사용을 위해 분리한 코드를 page.tsx파일과 동일한 위치에 두었는데
해당 파일을 component/ 아래로 옮겨야 하는지에 대한 의견 주시면 반영하겠습니다 :)

slinky-t

This comment was marked as duplicate.

Copy link
Contributor

@dladncks1217 dladncks1217 left a comment

Choose a reason for hiding this comment

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

일단 Interaction이 필요해서 'use client' 사용을 위해 분리한 코드를 page.tsx파일과 동일한 위치에 두었는데 해당 파일을 component/ 아래로 옮겨야 하는지에 대한 의견 주시면 반영하겠습니다 :)

저는 component하위로 옮기는게 좋을 것 같긴 합니다!


소소하게 리뷰 + 궁금한거 몇개 남겨봤습니당
확인 부탁드릴게요~!

src/api/onboard.tsx Outdated Show resolved Hide resolved
src/api/onboard.tsx Outdated Show resolved Hide resolved
import LinkedInIcon from '@/assets/icons/LinkedInIcon';
import { EmailSenderButton } from '@/components/EmailSenderButton';
import { OnboardButton } from '@/components/OnboardButton';
import { IncomingSenders } from '@/types/onboard';
Copy link
Contributor

Choose a reason for hiding this comment

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

import type으로 받아오는 방법도 있긴 합니다!
따로 정하지 않긴 해서 ㅎㅎ...
어떤게 좋을거같나요?

Copy link
Contributor

Choose a reason for hiding this comment

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

네이밍이 딱 읽었을 때 타입같은 느낌이 부족한 것 같은데, 어떻게 생각하시나요!?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

좋아요 👍 type관련 부분은 import type으로 불러오도록 할게요 :)

@@ -0,0 +1,156 @@
import ServiceIcon from '@/assets/icons/ServiceIcon';
import EmailListInteraction from './interaction';
Copy link
Contributor

Choose a reason for hiding this comment

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

절대경로 안들어갔어요! + 밑에 더있어용

userName: string;
}

const EmailList = async ({ userName = '채현' }: EmailListProps) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

userName optional 아니다보니 default값 안넣어줘도 될듯요!?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

이 부분이 원래는 api를 통해 userData를 받아와야하는 부분인데, api의 연결 이슈로 인해 일단 UI를 보여주기 위해 넣어놓았어요! API 연결하면서 해당 부분을 수정하도록 할게요!


export default EmailList;

const getIncomingSenders = async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

목데이터라서 이후에 api 호출 코드 들어갈거라서 async로 미리 선언해둔건가요!?


export default Interest;

async function getInterest() {
Copy link
Contributor

Choose a reason for hiding this comment

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

위에서 const로 선언했었는데, 여기서는 function키워드라서 그런데, 통일하는게 좋을 것 같아서요!
어떤게 더 좋나요?

export default Interest;

async function getInterest() {
// const interestList = await getInterestList();
Copy link
Contributor

Choose a reason for hiding this comment

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

불필요한 주석들은 지워주시는게 좋을것같아요!

@@ -60,7 +60,7 @@ const ServiceIcon = ({ width, height, className, onClick }: IconProps) => {
d='M46.3492 12.8549C46.7575 12.8571 48.8776 12.916 50.5706 14.6356C52.3889 16.4854 52.2985 18.8225 52.2769 19.1953L52.2723 19.2746H52.3518H54.0336H54.1086V19.1996C54.1086 18.8315 54.1566 16.5184 56.0635 14.7902C57.8881 13.1379 60.0621 13.2461 60.4445 13.2721L60.5246 13.2775V13.1972V11.5237V11.4487H60.4496C60.0759 11.4487 57.8543 11.4136 56.1334 9.58784C54.3908 7.73813 54.5007 5.47498 54.5266 5.10484L54.5322 5.02461H54.4518H52.746H52.6718L52.671 5.09875C52.6666 5.47805 52.6035 7.72617 50.7504 9.44814C48.9276 11.1397 46.7382 11.0576 46.3538 11.0359L46.2746 11.0314V11.1108V12.7799V12.8545L46.3492 12.8549ZM54.8607 13.4186C54.0927 14.1128 53.6048 14.8377 53.3067 15.3752C53.0726 14.8627 52.6761 14.1594 52.0212 13.4579L52.0211 13.4578C51.3624 12.7545 50.6815 12.3118 50.1857 12.0445C50.676 11.7997 51.2954 11.4251 51.9203 10.8612L51.9203 10.8611C52.6841 10.1709 53.1762 9.45231 53.4804 8.91494C53.7304 9.43698 54.1456 10.1476 54.8203 10.8508C55.4632 11.5221 56.1191 11.9564 56.6123 12.2291C56.1243 12.4693 55.5087 12.836 54.8857 13.3939L54.8607 13.4163V13.4186Z'
fill='black'
stroke='black'
stroke-width='0.15'
strokeWidth='0.15'
Copy link
Contributor

Choose a reason for hiding this comment

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

어떤 이슈가 있었나요!?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

react에서는 kebab case가 아닌 camel case로 (stroke-width가 아닌 strokeWidth로) 써야하는것으로 알고있어요!
image

Copy link
Contributor

Choose a reason for hiding this comment

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

아 생각해보니 svg였군요?
제가 시간날때 한번 래핑해서 쓰기 편하게 만들어볼게용

//withCredentials: true,
headers: {
// "X-AUTH-TOKEN": getCookie("userToken"),
// Authorization: getCookie('userToken'),
Copy link
Contributor

Choose a reason for hiding this comment

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

클라에서 토큰 최대한 핸들링 안하고싶은데 어떻게 될지 모르겠네요 ㅎㅎ...😅

https://www.youtube.com/live/RQv86D0M5YY?si=SlIS954YtoaFzddI&t=4804
https://www.youtube.com/live/RQv86D0M5YY?si=3AIVIT5oJdcf-fNj&t=5467
https://cheatsheetseries.owasp.org/cheatsheets/HTML5_Security_Cheat_Sheet.html
스크린샷 2024-07-07 오후 7 19 52

암호화가 아니라 인코딩된 식별자다보니 클라측 메모리에서 갖고있는건 좀 피하고싶어서... (백에서 csrf토큰 추가하는일 있더라도..)

httpOnly에 secure달린 쿠키 credentials헤더달아서 브라우저가 처리하게 하고싶긴 한데 백엔드랑 얘기가 어케될지 모르겠네요🥲

Copy link
Contributor Author

Choose a reason for hiding this comment

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

백엔드 해줘

@moong23 moong23 requested a review from dladncks1217 July 10, 2024 13:40
@moong23
Copy link
Contributor Author

moong23 commented Jul 10, 2024

늦은 시간에 시간에 쫓겨서 작업하다보니 부족한 부분이 조금 많은 것 같네요 :(
조금 더 정신 차리고 작업해보겠습니다 ㅎㅎ

@dladncks1217
Copy link
Contributor

고생하셨습니다~!

@moong23 moong23 merged commit 70cb95d into dev Jul 10, 2024
@moong23 moong23 deleted the feat/onboard branch July 10, 2024 14:18
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.

3 participants