Skip to content

Conversation

@happyhyep
Copy link
Contributor

@happyhyep happyhyep commented Sep 23, 2024

안녕하세요!
메인 화면에 이런 식으로 단어 단위로 줄바꿈이 되지 않아 UX적으로 좋지 않다는 생각이 들어 수정해보았습니다!

image

아래 일본어 페이지를 참고하였고,

image

이런 식으로 수정하였습니다.

image

번역이 아니지만 개선해도 되는 부분이 맞다면 다른 페이지와 내용도 개선해보겠습니다!

감사합니다!

Progress

@vercel
Copy link

vercel bot commented Sep 23, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
ko-legacy-reactjs-org ⬜️ Ignored (Inspect) Visit Preview Sep 24, 2024 7:36am

@github-actions
Copy link

github-actions bot commented Sep 23, 2024

Size changes

📦 Next.js Bundle Analysis for react-dev

This analysis was generated by the Next.js Bundle Analysis action. 🤖

⚠️ Global Bundle Size Increased

Page Size (compressed)
global 104.2 KB (🟡 +4 B)
Details

The global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

If you want further insight into what is behind the changes, give @next/bundle-analyzer a try!

Five Pages Changed Size

The following pages changed size from the code in this PR compared to its base branch:

Page Size (compressed) First Load
/404 106.45 KB (🟡 +87 B) 210.65 KB
/500 106.44 KB (🟡 +87 B) 210.63 KB
/[[...markdownPath]] 108.47 KB (🟡 +87 B) 212.67 KB
/errors 106.56 KB (🟡 +87 B) 210.76 KB
/errors/[errorCode] 106.54 KB (🟡 +87 B) 210.73 KB
Details

Only the gzipped size is provided here based on an expert tip.

First Load is the size of the global bundle plus the bundle for the individual page. If a user were to show up to your website and land on a given page, the first load size represents the amount of javascript that user would need to download. If next/link is used, subsequent page loads would only need to download that page's bundle (the number in the "Size" column), since the global bundle has already been downloaded.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

Next to the size is how much the size has increased or decreased compared with the base branch of this PR. If this percentage has increased by 10% or more, there will be a red status indicator applied, indicating that special attention should be given to this.

Copy link
Collaborator

@lumirlumir lumirlumir left a comment

Choose a reason for hiding this comment

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

안녕하세요👍 좋은 제안입니다만 몇가지 수정 요청사항이 있습니다.

  1. .idea 디렉토리 내부의 내용은 반영 불가합니다. 삭제 부탁드립니다.
  2. 아래 부분도 함께 수정되면 좋지 않을까 합니다.
    image
    image
  3. 코드 구현 관련해서 수정 요청사항이 있습니다.

@lumirlumir lumirlumir changed the title fix: ux improvement fix: HomeContent ux improvement Sep 23, 2024
@happyhyep
Copy link
Contributor Author

넵 감사합니다! 혹시 수정하였는데, 확인해주실 수 있으실까요? @lumirlumir

@lumirlumir
Copy link
Collaborator

lumirlumir commented Sep 23, 2024

헉 현재 반응형에서 개행이 잘못된 위치에 발생하는 경우와 띄어쓰기가 안되는 경우가 생기네요...ㅠㅠ

  1. 노트북 1024px

image

  1. 768px 이하

image

@lumirlumir lumirlumir self-requested a review September 23, 2024 06:30
@lumirlumir lumirlumir changed the title fix: HomeContent ux improvement perf: HomeContent ux improvement Sep 23, 2024
@happyhyep
Copy link
Contributor Author

넵 감사합니다!!
말씀해주신대로 Pr 태그에 props 추가해보았는데, 이렇게 하면 1223px ~ 1279px에서 반응형이 깨지는 문제가 발생하는 것을 확인하였습니다. 그래서 다른 방식으로 수정해보았는데, 반응형에는 문제가 없는 것 같아 푸시하였습니다!
확인 부탁드리겠습니다 감사합니다! 😊 @lumirlumir

@lumirlumir
Copy link
Collaborator

lumirlumir commented Sep 23, 2024

@happyhyep 수고하셨습니다🙇‍♂️. 아래 내용 조금만 더 반영해주셔서 커밋해주시면 좋을 것 같습니다.

첫째로, 1221px ~ 1279px에서 문장 길이와 tailwindCSS의 반응형이 맞물리지 않아 아래와 같이 글자가 넘어가는 현상이 있는 것은 확인했습니다.

image

다만, tailwindCSS 규격에 맞지 않는 너무 지엽적인 사이즈까지 고려하여 컴포넌트를 구성하는 것은 과하다고 개인적으로 생각됩니다. 이는 종종 글 자체 내용이 수정될 경우도 발생하기에, 매번 컴포넌트 반응형 사이즈를 조절하는 것은 비효율적이라 그렇습니다. 따라서, 위 코드 제안에서와 같이 tailwind가 제공하는 반응형 사이즈 안에서만 조절하는 것을 권장합니다.

둘째로, <Br> 컴포넌트에 tailwindCSS 속성을 다 빼버리면, 작은 사이즈의 반응형에서 강제 개행이 발생해 버립니다. 아래와 같이 말이죠. 작은 사이즈에서는 개행 없이 가는 것이 더 좋은 것 같습니다.

image

ja.react.dev에서도 큰 틀에서의 개행만 진행하며, 너무 세세한 부분의 개행까지는 신경쓰지 않기에 결론적으로, 제가 코드 리뷰 해드린 방향으로 수정하시는 것을 권장드립니다.

Comment on lines 69 to 71
function Br() {
return <br />;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
function Br() {
return <br />;
}
function Br({breakPointPrefix = 'lg'}) {
const breakPointClass = {
lg: 'lg:inline',
xl: 'xl:inline',
}[breakPointPrefix];
return <br className={cn('hidden', breakPointClass)} />;
}

이렇게 변경하는 것을 권장합니다.

<Center>
<Header>컴포넌트를 사용하여 사용자 인터페이스 만들기</Header>
<Header>
컴포넌트를 사용하여 <Br /> 사용자 인터페이스 만들기
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
컴포넌트를 사용하여 <Br /> 사용자 인터페이스 만들기
컴포넌트를 사용하여
<Br /> 사용자 인터페이스 만들기

얘도 일관성을 맞춰 수정하는게 좋을 듯 합니다

Comment on lines 434 to 435
새로운 기능에 맞춰
<Br /> 업그레이드 하기
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
새로운 기능에 맞춰
<Br /> 업그레이드 하기
새로운 기능에 맞춰
<Br breakPointPrefix="xl" /> 업그레이드 하기

@happyhyep
Copy link
Contributor Author

헉 넵 정말 감사합니다!! 댓글 드린다는 걸 깜박했네요 😂
말씀하신 내용으로 수정했습니다! 감사합니다 !! @lumirlumir

Comment on lines 73 to 74
}[breakPointPrefix];
return <br className={cn('hidden', breakPointClass)} />;
Copy link
Collaborator

@lumirlumir lumirlumir Sep 24, 2024

Choose a reason for hiding this comment

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

Suggested change
}[breakPointPrefix];
return <br className={cn('hidden', breakPointClass)} />;
}[breakPointPrefix];
return <br className={cn('hidden', breakPointClass)} />;

여기 사이에 공백 하나만 넣어주세요~

Copy link
Contributor Author

Choose a reason for hiding this comment

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

넵 추가했습니다! 감사합니다 ! @lumirlumir

Copy link
Collaborator

@lumirlumir lumirlumir left a comment

Choose a reason for hiding this comment

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

LGTM

@lumirlumir lumirlumir merged commit d30f992 into reactjs:main Sep 24, 2024
4 checks passed
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