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

feature: first page markup #23

Open
wants to merge 2 commits into
base: petit-project/team3
Choose a base branch
from

Conversation

dev-owen
Copy link
Member

/wonjong

첫번째 페이지 마크업 부분 완성된 데까지 PR 올립니다.

2022-06-26.1.07.38.mov

.

Copy link

@wooogi123 wooogi123 left a comment

Choose a reason for hiding this comment

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

혹시 AnimatedText 컴포넌트가 공백이 없는 문자열만 받게 처리하고 싶으시다면

type NoSpaceString<T extends string> = T extends `${string} `
  ? never
  : T extends ` ${string}`
  ? never
  : T extends ` ${string} `
  ? never
  : T extends `${string} ${string}`
  ? never
  : string;

요런 타입으로도 사전에 막을 수 있을 것 같습니다.

import * as $ from './styles'

// Word wrapper
const Wrapper = (props: any) => {

Choose a reason for hiding this comment

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

TypeScript 환경이기 때문에 해당 부분의 props을 타입 정의해주는 것이 좋을 것 같습니다.
해당 컴포넌트의 경우 { children: React.ReactNode }로 정의할 수 있을 것 같습니다.

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 16 to 27
const item = {
hidden: {
y: "200%",
color: "#FFFFFF",
transition: { ease: [0.455, 0.03, 0.515, 0.955], duration: 0.85 }
},
visible: {
y: 0,
color: "#FFFFFF",
transition: { ease: [0.455, 0.03, 0.515, 0.955], duration: 0.75 }
}
};

Choose a reason for hiding this comment

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

AnimatedText 컴포넌트 함수 내부에서 정의할 필요 없이, 컴포넌트 선언 밖으로 뽑아도 좋을 것 같습니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

컴포넌트 선언 밖으로 뽑는 것과 안에 선언하는 것에 어떤 차이가 있을까요? 이 부분은 제가 생각하기에는 마이너한 부분인 것 같은데, 혹시 코멘트 주신 이유가 있을 수도 있어 보여서 질문드렸습니다.

Choose a reason for hiding this comment

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

마이너한 부분은 맞습니다. 다만 함수 내부에 정적 변수가 선언되어서 이후 해당 컴포넌트가 계산될 때 마다 변수가 생성될텐데, 불필요하게 변수를 생성시킬 필요가 없이 함수 외부에서 한번만 생성할 수 있을 것 같아 코멘트 남겨놓았습니다!

Comment on lines 29 to 43
// Split each word of props.text into an array
const splitWords = props.text.split(" ");

// Create storage array
const words: string[][] = [];

// Push each word into words array
for (const [, item] of splitWords.entries()) {
words.push(item.split(""));
}

// Add a space ("\u00A0") to the end of each word
words.map((word) => {
return word.push("\u00A0");
});

Choose a reason for hiding this comment

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

해당 부분의 가공 로직이 너무 찢어져있는 것 같은데, method chaining을 통해 정리할 수도 있을 것 같습니다.

const words = props.text.split(" ")
  .map((el) => el.split(""))
  .map((el) => [...el, "\u00A0"]);

요런식으로 정리하는 것도 좋을 것 같습니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

함수형으로 정리하면 훨씬 더 깔끔하네요! 반영하도록 하겠습니다.

return (
// Wrap each word in the Wrapper component
<Wrapper key={index}>
{words[index].flat().map((element, index) => {

Choose a reason for hiding this comment

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

요 부분 flat().map() 해당 구분이랑 flatMap()과는 다른 동작이 진행되나요?

Copy link
Member Author

Choose a reason for hiding this comment

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

flapMap 메서드는 제가 몰랐었는데 한 번 알아볼께요

}}
>
<AnimatedText text={String(randomNumbers[2])}
style={{ fontSize: '60px', fontWeight: '900', textAlign: 'right', margin: '0 40px 0 0' }} />

Choose a reason for hiding this comment

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

정의되어있지 않은 style을 props으로 받아서 사용중인 상태라 수정이 필요할 것 같습니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

네 수정하도록 할께요

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.

2 participants