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

[Landing Renewal] Mode View Slider 추가 #96

Merged
merged 18 commits into from
Jul 2, 2024

Conversation

sumi-0011
Copy link
Member

@sumi-0011 sumi-0011 commented Jun 30, 2024

💡 기능

  • mode 보여주는 slider를 추가하였습니다.
  • @hyesungoh 의견대로 naver의 Flicking 라이브러리를 이용하여 구현하였어요.
  • Flicking 슬라이더를 component 에 두고 래핑하여 사용할까? 하다가, ref를 이용해 가져와야하는 것들이 많아. 분리하지 않았습니다.

🔎 기타

Jun-30-2024 17-30-32

# Conflicts:
#	apps/web/src/app/page.tsx
@sumi-0011 sumi-0011 changed the base branch from main to feat/renewal-landing June 30, 2024 09:13
@devxb
Copy link
Member

devxb commented Jun 30, 2024

이거 안의 이미지를 동영상으로 넣으려고 했는데, 깜빡하고 전달을 안드렸네요 🙏 (cc. @Youna-Ha)
이미지 -> 동영상 변환에 시간이 오래걸리면 이렇게 해도 괜찮을거 같아요, (변경에 공수가 적다면 동영상으로 부탁드리겠습니다)

Copy link
Member

@hyesungoh hyesungoh left a comment

Choose a reason for hiding this comment

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

이미지만 webp 확장자로 사용해주시면 좋을 거 같아용 👍 👍

추가적으로 정하고 싶은 컨벤션도 있는데 다음과 같아요


1. 스타일 동일 파일 내 위치 vs .style.ts or .css.ts와 같은 파일로 분리

import * as s from './foo.style.ts`

<div calssName={s.fooDiv} >

이런 식으로 스타일에 대한 것들을 한 파일로 넣을 수도 있을 거 같은데,
이런 방식은 어떻게 생각하시는 지 궁금해용


2. () => void vs VoidFunction

두 타입에 대한 차이점은 없지만, 맞춰보면 좋을 거 같아용

Copy link
Member

Choose a reason for hiding this comment

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

이미지는 png보다 webp로 export해서 사용하면 어떨까용??

피그마에 플러그인있어용

Copy link
Member Author

Choose a reason for hiding this comment

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

피그마에 플러그인이 있다구요??! 이건 몰랐네요 좋은 정보 감사합니다
webp를 우선 로드하도록 변경해둘게요

Copy link
Member

Choose a reason for hiding this comment

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

흠흠 아이콘을 어떻게 할 지 고민이군요

Copy link
Member Author

Choose a reason for hiding this comment

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

이게 진짜 고민되는 부분인것 같아요...
계속 고민하면서 미루게 된달까..

Comment on lines 9 to 15
return (
<div>
<section className={modeDemoSectionStyle}>
<LandingMainSlider />
</section>
</div>
);
Copy link
Member

Choose a reason for hiding this comment

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

겉의 div는 필요없는 위계일 수 있을 거 같은데, 추가하신 이유가 무엇인가용?

Copy link
Member Author

Choose a reason for hiding this comment

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

이후에 다른 섹션들이 추가될 예정이기때문에 고려해서 추가해두었습니다!

Copy link
Member

Choose a reason for hiding this comment

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

얘의 역할은 무엇인가요?

Copy link
Member Author

Choose a reason for hiding this comment

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

해당 파일 말씀하시는거죠?!
이후에 슬라이드 뿐만이 아니라 다른 텍스트들이 추가되어야하는 부분이라 필요한 파일이라고 생각했어요.
그런데 이름을 잘못 지은 것 같네요..

이후에 요기 PR의 welcome section 파일로 합쳐지게 될것 같아요

margin: 'auto',
});

const prevArrowStyle = cx(arrowStyle, css({ left: '-62px' }));
Copy link
Member

Choose a reason for hiding this comment

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

오호 이렇게 사용할 수 있군용 👍

Comment on lines +157 to +184
function ArrowButton({
onClick,
direction,
disabled,
}: {
onClick: () => void;
direction: 'prev' | 'next';
disabled: boolean;
}) {
return (
<button
onClick={onClick}
className={cx(
direction === 'prev' ? prevArrowStyle : nextArrowStyle,
css({
rotate: direction === 'prev' ? '180deg' : '0deg',
cursor: disabled ? 'not-allowed' : 'pointer',
}),
)}
>
{disabled ? (
<Image src="/icon/circle-arrow-disable.svg" alt="arrow" width={36} height={36} />
) : (
<Image src="/icon/circle-arrow.svg" alt="arrow" width={40} height={40} />
)}
</button>
);
}
Copy link
Member

Choose a reason for hiding this comment

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

이 애로우 버튼 컴포넌트는 서버 컴포넌트로 사용할 수 있을 거 같네요 🤔

@sumi-0011
Copy link
Member Author

sumi-0011 commented Jul 1, 2024

이미지만 webp 확장자로 사용해주시면 좋을 거 같아용 👍 👍

추가적으로 정하고 싶은 컨벤션도 있는데 다음과 같아요

1. 스타일 동일 파일 내 위치 vs .style.ts or .css.ts와 같은 파일로 분리

import * as s from './foo.style.ts`

<div calssName={s.fooDiv} >

이런 식으로 스타일에 대한 것들을 한 파일로 넣을 수도 있을 거 같은데, 이런 방식은 어떻게 생각하시는 지 궁금해용

2. () => void vs VoidFunction

두 타입에 대한 차이점은 없지만, 맞춰보면 좋을 거 같아용

  1. 좋습니다!
    개인적으로 css 보다는 .style.ts이라는 네이밍을 사용하면 좋을 것 같아요.

  2. VoidFunction보다는 () => void를 선호합니다.
    제가 알고있기로는 조금 차이가 있는것으로 알고있습니다.
    그래서 저는 () => void로 통일하는 것을 선호하는데, 혜성님은 어떻게 생각하시나요


GPT 대답에 의하면

TypeScript에서 () => void와 VoidFunction은 유사하지만 약간의 차이가 있습니다.

() => void:

() => void는 함수 타입 애노테이션입니다. 이는 "아무 인수도 받지 않고, 아무것도 반환하지 않는 함수"를 의미합니다.
예시: const myFunction: () => void = () => { console.log("Hello World"); };

VoidFunction:

VoidFunction은 TypeScript에서 미리 정의된 타입입니다. 이는 () => void와 비슷하지만, 함수의 반환값을 무시하는 특성이 있습니다.
정확히는, VoidFunction 타입은 함수가 반환값을 가질 수는 있지만 그 반환값이 사용되지 않는다는 것을 의미합니다.

type VoidFunction = (...args: any[]) => void;

차이점 정리:

() => void는 반환값이 없음을 명시합니다.
VoidFunction은 반환값이 있을 수 있지만, 그 반환값이 사용되지 않음을 명시합니다.
이 두 타입의 차이는 주로 함수의 반환값을 무시할 때 유용합니다. 예를 들어, 어떤 API에서 콜백 함수의 반환값을 신경 쓰지 않는다면 VoidFunction을 사용하여 반환값이 무시된다는 것을 명확히 할 수 있습니다.

예시:

const example1: () => void = () => {
    console.log("This returns nothing");
};

const example2: VoidFunction = () => {
    return "This returns a string, but the return value is ignored";
};

이처럼 VoidFunction을 사용하면 반환값이 있는 함수를 정의할 수 있지만, 그 반환값이 사용되지 않음을 보장할 수 있습니다. 반면 () => void는 반환값이 아예 없음을 명시합니다.

@hyesungoh
Copy link
Member

@sumi-0011 흠흠 저는 그 대답이 할루시네이션이라고 생각하는데요!!

왜냐하면 () => void도 return 값이 있을 때 에러를 발생시키지 않기 떄문이에용

@sumi-0011
Copy link
Member Author

@sumi-0011 흠흠 저는 그 대답이 할루시네이션이라고 생각하는데요!!

왜냐하면 () => void도 return 값이 있을 때 에러를 발생시키지 않기 떄문이에용

헐..! 그렇긴 하네요 GPT가 또.?
그럼 둘 방식 모두 큰 상관 없을거같은데, () => void는 import 안해도 되서 좀더 편하지 않을까 하는 생각이 었어요
어떤 방식으로 통일할까요?? 의견에 따를게용

@sumi-0011 sumi-0011 linked an issue Jul 2, 2024 that may be closed by this pull request
9 tasks
@hyesungoh
Copy link
Member

@sumi-0011 흠흠 저는 그 대답이 할루시네이션이라고 생각하는데요!!
왜냐하면 () => void도 return 값이 있을 때 에러를 발생시키지 않기 떄문이에용

헐..! 그렇긴 하네요 GPT가 또.? 그럼 둘 방식 모두 큰 상관 없을거같은데, () => void는 import 안해도 되서 좀더 편하지 않을까 하는 생각이 었어요 어떤 방식으로 통일할까요?? 의견에 따를게용

VoidFunction도 타입스크립트 내장 타입이라, 임포트 필요없어요!

각 장점이라면 저는 이렇게 느껴지는데 ...

VoidFunction - 읽기 쉽다 (단어라서)
() => void - 이후 인터페이스 수정에 손가락을 덜 움직여도 된다

둘 다 메이저한 장점이라고는 생각 안돼서, 저도 상관없습니다 ~

@sumi-0011 sumi-0011 requested a review from hyesungoh July 2, 2024 12:50
@sumi-0011
Copy link
Member Author

@sumi-0011 흠흠 저는 그 대답이 할루시네이션이라고 생각하는데요!!
왜냐하면 () => void도 return 값이 있을 때 에러를 발생시키지 않기 떄문이에용

헐..! 그렇긴 하네요 GPT가 또.? 그럼 둘 방식 모두 큰 상관 없을거같은데, () => void는 import 안해도 되서 좀더 편하지 않을까 하는 생각이 었어요 어떤 방식으로 통일할까요?? 의견에 따를게용

VoidFunction도 타입스크립트 내장 타입이라, 임포트 필요없어요!

각 장점이라면 저는 이렇게 느껴지는데 ...

VoidFunction - 읽기 쉽다 (단어라서) () => void - 이후 인터페이스 수정에 손가락을 덜 움직여도 된다

둘 다 메이저한 장점이라고는 생각 안돼서, 저도 상관없습니다 ~

카톡으로 이야기한 것 과 같이 VoidFunction으로 통일하기로 해요!
그리고 다른 함수들도 (foo: number) => number -> type FooFunction과 같이 형식을 통일하는 거로 합시다 땅땅당

@sumi-0011 sumi-0011 merged commit 6bc6368 into feat/renewal-landing Jul 2, 2024
5 checks passed
@sumi-0011 sumi-0011 deleted the feat/main-slider branch July 2, 2024 12:52
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.

Flicking 라이브러리를 사용하여 슬라이더 구현
3 participants