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

[김희진] sprint8 #111

Conversation

devmanta
Copy link
Collaborator

요구사항

기본

  • 스프린트 미션 1 ~ 7에 대해 typescript를 적용해주세요

심화

  • any타입을 최소한으로 써주세요

@devmanta devmanta self-assigned this Oct 19, 2024
@devmanta devmanta added the 매운맛🔥 뒤는 없습니다. 그냥 필터 없이 말해주세요. 책임은 제가 집니다. label Oct 19, 2024
Comment on lines +14 to +16
const [preview, setPreview] = useState<string | null>(null);
const [error, setError] = useState<string | null>(null);
const inputRef = useRef<HTMLInputElement | null>(null);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

초기값을 Null로 하고싶으면 이렇게 계속 | null 해줘야하는거 맞나요..?

Copy link
Collaborator

Choose a reason for hiding this comment

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

넵 만약 File | undefined와 같은 경우라면 file?: File처럼 작성하셔도 되지만 null이라면 따로 유틸리티 타입을 만드시거나 File | null 이렇게 작성해주셔야 합니다~

const handleOnClick = () => {
if (onDelete) onDelete(index);
if (onDelete && index !== undefined) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

index에 undefined 체크 안하면 오류나서 넣긴했는데 더 좋은방법 있을까요?

Copy link
Collaborator

Choose a reason for hiding this comment

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

onDelete 함수의 인자값인 index가 필수값이라 빈값인지 확인을 해줘야 합니다~
다른 방식으로 작성하시고 싶으시면 아래처럼 작성할 수 있습니다.

  const handleOnClick = () => {
    if (onDelete && index) {
      onDelete(index);
    }
  };
  const handleOnClick = () => {
    if (!index) return;
    onDelete?.(index);
  };

Copy link
Collaborator

Choose a reason for hiding this comment

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

다만 타입을 명시하니 의문이 하나 생기네요~
tagCard에 index가 없는 경우가 있을까요?
index를 필수값으로 변경하시는 것이 더 옳은 방향이 아닐까 생각이 듭니다~

Comment on lines +25 to +29
const queryParams = new URLSearchParams({
page: page.toString(),
pageSize: pageSize.toString(),
orderBy,
});
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

이부분은 이렇게 해야지 오류가 없어지긴 하던데..
이렇게 하는게 아닌것같은 느낌입니다.
혹시 다른 방법이 있을까요?

Copy link
Collaborator

Choose a reason for hiding this comment

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

어떤 이유로 그렇게 생각하셨을까요?
URLSearchParams의 인자의 경우 string 의 형태로 넘겨줘야 하기에 page, pageSize 같은 number 값들은 변환을 해주어야 합니다~

package.json Show resolved Hide resolved
Copy link
Collaborator

@GANGYIKIM GANGYIKIM left a comment

Choose a reason for hiding this comment

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

희진님 이번 스프린트 미션 고생하셨습니다~

type?: string;
label?: string;
inputId?: string;
name: string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

P2:
가능하면 필수값을 위에 두시는 것을 추천드립니다.

interface InputBarProps {
  name: string;
  type?: string;
  label?: string;
  inputId?: string;
  value?: string;
  placeholder?: string;
  disabled?: boolean;
  onChange?: (event: React.ChangeEvent<HTMLInputElement>) => void;
  onKeyDown?: (event: React.KeyboardEvent<HTMLInputElement>) => void;
}

return (
<section className="InputBar">
{label && (
<label htmlFor={inputId} className="InputBar-label">
<label htmlFor={inputId || id} className="InputBar-label">
Copy link
Collaborator

Choose a reason for hiding this comment

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

P1:
하단의 input도 inputId가 없을 시 id 값을 가지도록 변경해주세요~

const eventProps = {
...(onKeyDown && { onKeyDown }),
...(onChange && { onChange }),
};

const id = uuidv4();
Copy link
Collaborator

Choose a reason for hiding this comment

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

P3:
react의 useId 훅을 사용하셔도 됩니다.

https://ko.react.dev/reference/react/useId

Comment on lines +25 to +29
const queryParams = new URLSearchParams({
page: page.toString(),
pageSize: pageSize.toString(),
orderBy,
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

어떤 이유로 그렇게 생각하셨을까요?
URLSearchParams의 인자의 경우 string 의 형태로 넘겨줘야 하기에 page, pageSize 같은 number 값들은 변환을 해주어야 합니다~

Comment on lines +3 to +16
interface PaginationButtonProps {
onClick?(event: React.MouseEvent<HTMLButtonElement>): void;
disabled?: boolean;
isActive?: boolean;
ariaCurrent?: 'page' | 'step' | undefined;
children: React.ReactNode;
}
function PaginationButton({
onClick,
disabled,
isActive,
ariaCurrent,
children,
}: PaginationButtonProps) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

P3:
아래처럼도 가능합니다.
잘 정리된 글이 있어서 공유드립니다. 다만 아래처럼 작성하게 되면 children 속성은 optional이라는 것을 유의해야 합니다.

interface PaginationButtonProps {
  onClick?(event: React.MouseEvent<HTMLButtonElement>): void;
  disabled?: boolean;
  isActive?: boolean;
  ariaCurrent?: 'page' | 'step'; // ? 마크가 있으므로 undefined를 명시할 필요가 없습니다.
}

function PaginationButton({
  onClick,
  disabled,
  isActive,
  ariaCurrent,
  children,
}: PropsWithChildren<PaginationButtonProps>) {

https://www.frontoverflow.com/question/11/React%20+%20TypeScript%EC%9D%98%20PropsWithChildren

Comment on lines +8 to +11
interface ItemProps
extends Pick<Product, 'id' | 'images' | 'name' | 'price' | 'favoriteCount'> {
imgSize: 'small' | 'large';
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

P3:
👍
다만 id, images, name, price, favoriteCount가 하나로 묶을 수 있다면 아래처럼 작성하셔도 좋을 것 같으니 참고해주세요~

export interface ProductBaseProps {
  id: number;
  images: string[];
  name: string;
  price: number;
  favoriteCount: number;
}

export interface Product extends ProductBaseProps { ... }

Comment on lines +6 to +7
interface CommentProps
extends Pick<CommentData, 'id' | 'content' | 'createdAt' | 'writer'> {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

P2:
Commnet에서 updateAt을 제외한 나머지가 필요하니 아래처럼 하셔도 좋을 것 같습니다.

Suggested change
interface CommentProps
extends Pick<CommentData, 'id' | 'content' | 'createdAt' | 'writer'> {}
interface CommentProps
extends Omit<CommentData, 'updatedAt'> {}

Comment on lines +14 to +16
const [preview, setPreview] = useState<string | null>(null);
const [error, setError] = useState<string | null>(null);
const inputRef = useRef<HTMLInputElement | null>(null);
Copy link
Collaborator

Choose a reason for hiding this comment

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

넵 만약 File | undefined와 같은 경우라면 file?: File처럼 작성하셔도 되지만 null이라면 따로 유틸리티 타입을 만드시거나 File | null 이렇게 작성해주셔야 합니다~

Comment on lines +4 to +14
interface TextareaProps {
label?: string;
inputId?: string;
name: string;
value: string;
placeholder?: string;
onChange?: (event: React.ChangeEvent<HTMLTextAreaElement>) => void;
disabled?: boolean;
heightSize?: string;
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

P2:
textarea의 속성과 input의 속성 중에 공유하는 것들이 많아 이를 고려해서 타입을 선언하셔도 좋을 것 같습니다.

Comment on lines +3 to +8
interface PrimaryRoundButtonProps {
children: React.ReactNode;
type?: 'button' | 'submit' | 'reset';
onClick?(event?: React.MouseEvent<HTMLButtonElement>): void;
disabled?: boolean;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

P1:
아래처럼 작성하시면 더 간결하고 의도가 보일 것 같습니다.

interface PrimaryButtonProps extends ButtonHTMLAttributes<HTMLButtonElement> {
  text: string;
}```

Copy link
Collaborator

Choose a reason for hiding this comment

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

그리고 button끼리 공통 타입도 만들어서 공유하면 좋겠습니다~

@GANGYIKIM GANGYIKIM merged commit 1d3ae61 into codeit-bootcamp-frontend:React-김희진 Oct 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
매운맛🔥 뒤는 없습니다. 그냥 필터 없이 말해주세요. 책임은 제가 집니다.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants