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

fix: useIsDarkMode의 초기값을 undefined로 설정 #212

Merged
merged 4 commits into from
Aug 16, 2024

Conversation

anonymousRecords
Copy link
Contributor

Overview

useIsDarkMode의 초기값을 undefined로 설정합니다.

기존의 방식은 다크 모드를 인식하기 전에 초기값 false를 노출시켜 오해를 일으킬 수 있다고 보았습니다.
따라서 undefined일 때는 null을 반환하는 방식으로 구현하여 모드가 제대로 인식되기 전에는 이미지가 노출되지 않도록 변경하였습니다.

예를 들어 화이트 모드일 경우,
1)다크 이미지가 불필요함에도 이미지를 가져오고 있고
2)이미지 변경이 일어나 깜박임 현상이 발생했습니다.

따라서 초기값을 undefined로 설정하여 불필요한 이미지를 가져오는 것을 없애고 이미지 노출을 막아 깜박임 현상을 해결하였습니다.

[변경 전]
as-is

[변경 후]
to-be

PR Checklist

  • I read and included theses actions below
  1. I have read the Contributing Guide
  2. I have written documents and tests, if needed.

Copy link

changeset-bot bot commented Aug 1, 2024

⚠️ No Changeset found

Latest commit: 0e3bbfe

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

vercel bot commented Aug 1, 2024

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

Name Status Preview Comments Updated (UTC)
es-hangul ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 16, 2024 6:35am

@okinawaa
Copy link
Member

okinawaa commented Aug 2, 2024

이슈레이징 감사해요!

  1. 처음부터 다크모드, 라이트모드 여부를 정확히 받아올 수 있는 방법은 없을까요?
  2. 현재 모든 컴포넌트에 아래와 같은 코드가 계속되어 사용될텐데, 이것을 놓치는 휴먼에러도 쉽사리 발생할 것 같아요. 혹시 더 공통화하여 사용할 수 있는 방법은 없을까요?
if (isDarkMode === undefined) {
    return null;
  }
  1. 기존에 깜빡이는 문제가 발생했던 영상같은것을 첨부해주실 수 있나요? 라이트 모드, 다크 모드 로 es-hangul docs를 들어가도 깜빡이는 현상은 크게 발생하지 않아서요. 어떠한 문제가 발생했는지 시각적으로 확인하고 싶습니다!

@anonymousRecords
Copy link
Contributor Author

anonymousRecords commented Aug 2, 2024

@okinawaa

  1. 처음부터 다크모드, 라이트모드 여부를 정확히 받아올 수 있는 방법은 없을까요?

cookie 등에 다크 모드를 저장해두는 방식으로 시도해 볼 수도 있겠지만...!
장기적인 관점에서 PNG 파일을 SVG 파일로 바꾸는 것이 더 효율적인 방안이라고 생각해요.

SVG 파일의 경우 currentColor를 주입하는 방식으로 동작하여 CSS 변수를 활용해서 해결이 가능합니다.
(참고 : Dark mode example with SSR and SSG, The Quest for the Perfect Dark Mode)

추가로 placeholder로 이미지 파일의 vector 값을 기반으로 추천한 이미지를 blurDataURL로 노출시키는 방안은, toss의 브랜드 이미지와도 연결된다고 판단해서 채택하지 않았습니다.


  1. 현재 모든 컴포넌트에 아래와 같은 코드가 계속되어 사용될텐데, 이것을 놓치는 휴먼에러도 쉽사리 발생할 것 같아요. 혹시 더 공통화하여 사용할 수 있는 방법은 없을까요?

말씀해주신 것처럼 현행 방식으로는 휴먼 에러가 발생할 수 있을 거 같아서, 하나의 컴포넌트로 해당 로직을 처리하는 게 좋을 것 같다고 생각이 들었어요.

저는 방법 1이 가장 좋다고 생각합니당! 이 부분 관련해서 @okinawaa 님 의견도 궁금합니다!!

[방법 1]

제 생각으로 직관성이 제일 좋다고 느꼈어요!

interface DarkModeProps {
  dark: React.ReactNode;
  light: React.ReactNode;
}

function DarkMode({ dark, light }: DarkModeProps) {
  const isDarkMode = useIsDarkMode();

  if (isDarkMode === undefined) {
    return null;
  }

  return isDarkMode ? dark : light;
}
    <DarkMode
      dark={<Image src="/logo-white.png" alt="logo" width="120" height="48" priority />}
      light={<Image src="/logo.png" alt="logo" width="120" height="48" priority />}
    />

[방법 2]

fallback 방식으로 다크모드를 기본 모드로 두고 라이트 모드를 대체(fallback)로 처리하는 방식이에요.

interface DarkModeProps {
  fallback: React.ReactNode;
  children: React.ReactNode;
}

function DarkMode({fallback, children} :DarkModeProps) {
  const isDarkMode = useIsDarkMode();

  if (isDarkMode === undefined) {
    return null;
  }

  return isDarkMode ? children : fallback;
}
    <DarkMode fallback={<Image src="/logo.png" alt="logo" width="120" height="48" priority />}>
      <Image src="/logo-white.png" alt="logo" width="120" height="48" priority />
    </DarkMode>

[방법3]

위 방법들(방법1,2)은 다크/화이트 모드라는 도메인에 밀접한 연관을 가지는 컴포넌트라고 생각해요. 물론 문서이기도 하고, 해당 기능의 경우 위 방식으로 마무리해도 상관은 없을 것 같지만 범용성을 챙겨보면 방법 3처럼 구현도 가능할 거라고 봅니다.

export const IfNotUndefined = ({ value, children }: RenderWhenProps) => {
  if (value === undefined) {
    return null;
  }
  return children;
};
  logo: function useLogo() {
    const isDarkMode = useIsDarkMode();

    return (
      <IfNotUndefined value={isDarkMode}>
          <Image src={isDarkMode ? "/logo-white.png" : "/logo.png"} alt="logo" width="120" height="48" priority />
      </RenderWhen>
    );
  },

  1. 기존에 깜빡이는 문제가 발생했던 영상같은것을 첨부해주실 수 있나요? 라이트 모드, 다크 모드 로 es-hangul docs를 들어가도 깜빡이는 현상은 크게 발생하지 않아서요. 어떠한 문제가 발생했는지 시각적으로 확인하고 싶습니다!
스크린샷 2024-08-02 오후 9 30 47 스크린샷 2024-08-02 오후 9 30 56

말씀해 주신 대로 실제로 육안으로 확인 가능한 깜박임 현상은 거의 없었습니다. 제가 설명을 부적절하게 했던 것 같아요...!

해당 PR이 반영되었을 때 아래와 같은 이점을 얻을 수 있습니다.

  1. 불필요한 이미지 로드 방지
  2. 렌더링 최적화
  3. 리소스 사용 개선

사용자 경험에 직접적으로 큰 영향을 미치지는 않지만, 코드의 효율성과 정확성을 높일 거라고 생각해요.

@okinawaa
Copy link
Member

okinawaa commented Aug 4, 2024

@anonymousRecords 님 정말 상세하게 설명해주셔서 감사해요.

**저도 1번방법이 가장 좋다고 생각합니다. **

DarkMode라는 컴포넌트의 인터페이스 자체는 좋은데, 컴포넌트 이름만 다크모드에 종속되지 않고, System Preference 의 맥락을 녹여낼 수 있으면 좋을 것 같아요. (ex. ThemeMode, ModeSwitcher, ModeToggler...)

또한 advance시킨다면, useIsDarkMode가 undefined일때 Layout shift를 방지할 수 있도록 fallback component등도 받으면 좋을 것 같다고 생각합니다!

@anonymousRecords
Copy link
Contributor Author

anonymousRecords commented Aug 5, 2024

@okinawaa
이전 피드백을 반영하여 코드를 수정했어요. 주요 변경사항은 다음과 같습니다.

  1. 컴포넌트 이름을 'DarkMode'에서 'ThemeMode'로 변경
  2. 단순히 이름을 바꾸는 것만으로는 다크 모드 종속성을 완전히 제거할 수 없다고 생각해서, 모드 결정 로직을 컴포넌트 외부로 분리
  3. fallback 옵션 추가 (fallback component로 어떤 걸 받아야)

추가적인 개선이 필요한 부분이 있다면 말씀해주세용

interface ThemeModeProps {
  dark: React.ReactNode;
  light: React.ReactNode;
  fallback?: React.ReactNode;
}

function ThemeMode({ dark, light, fallback = null }:ThemeModeProps) {
  const isDarkMode = useIsDarkMode();

  if (isDarkMode === undefined) {
    return fallback;
  }

  return isDarkMode ? dark : light;
}
logo: (
    <ThemeMode
      dark={<Image src="/logo-white.png" alt="logo" width="120" height="48" priority />}
      light={<Image src="/logo.png" alt="logo" width="120" height="48" priority />}
    />
 )

@okinawaa
Copy link
Member

okinawaa commented Aug 9, 2024

늦게 답변 드려 죄송합니다
제안주신 ThemeMode좋은 것 같아요. fallback component로 로더나 스켈레톤같은것을 제공해줄 수 있겠네요.

@okinawaa
Copy link
Member

okinawaa commented Aug 9, 2024

수정사항이 반영이 안된것 같은데, 혹시 브랜치에 반영이 되었을까요?

@anonymousRecords
Copy link
Contributor Author

@okinawaa
피드백 받고 반영하려고 아직 안 했다가, 반영해두었습니다.

그런데 언급한 ThemeMode의 경우, Sandpack 컴포넌트에 적용해보니 중복된 코드가 너무 발생하는 거 같아서 다시 수정해보았습니다!

interface ThemeModeProps {
  children: (theme: 'dark' | 'light') => React.ReactNode;
  fallback?: React.ReactNode;
}

export function ThemeMode({ children, fallback = null }: ThemeModeProps) {
  const isDarkMode = useIsDarkMode();

  if (isDarkMode == null) {
    return fallback;
  }

  return children(isDarkMode ? 'dark' : 'light');
}

기존에 useIsDarkMode 사용하던 코드와 유사하게 사용할 수 있는 거 같아서,

  • 해당 코드로 ThemeMode 컴포넌트 수정
  • ThemeMode 적용

까지 해서 반영해두었습니당

@codecov-commenter
Copy link

codecov-commenter commented Aug 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.83%. Comparing base (3c5c7f6) to head (0e3bbfe).
Report is 42 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #212      +/-   ##
==========================================
+ Coverage   95.34%   99.83%   +4.49%     
==========================================
  Files          18       31      +13     
  Lines         322      600     +278     
  Branches       77      145      +68     
==========================================
+ Hits          307      599     +292     
+ Misses         14        1      -13     
+ Partials        1        0       -1     

Copy link
Member

@okinawaa okinawaa left a comment

Choose a reason for hiding this comment

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

감사합니다 !! 🙇

@okinawaa okinawaa merged commit 1b62156 into toss:main Aug 16, 2024
2 of 10 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.

3 participants