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

feat(react): useDarkMode 신규 훅 추가 #489

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

ssi02014
Copy link
Contributor

@ssi02014 ssi02014 commented Sep 27, 2024

Overview

feat(react): useDarkMode 신규 훅 추가

useDarkMode는 기본적으로는 prefers-color-scheme를 기반으로 사용자 운영체제에 설정된 화면 모드로 dark모드인지 light모드인지 판단합니다.

만약, 반환하는 set 함수들을 호출하게 되면 로컬 스토리지에 여부를 저장하며, 스토리지에 값이 존재한다면 해당 값을 기반으로 dark모드인지 ligth 모드인지 판단합니다.

PR Checklist

  • All tests pass.
  • All type checks pass.
  • I have read the Contributing Guide document.
    Contributing Guide

@ssi02014 ssi02014 added feature 새로운 기능 추가 @modern-kit/react @modern-kit/react labels Sep 27, 2024
@ssi02014 ssi02014 self-assigned this Sep 27, 2024
Copy link

changeset-bot bot commented Sep 27, 2024

🦋 Changeset detected

Latest commit: efd9260

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@modern-kit/react Minor

Not sure what this means? Click here to learn what changesets are.

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

Copy link

codecov bot commented Sep 27, 2024

Codecov Report

Attention: Patch coverage is 12.50000% with 21 lines in your changes missing coverage. Please review.

Project coverage is 96.17%. Comparing base (33cf068) to head (efd9260).
Report is 13 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #489      +/-   ##
==========================================
- Coverage   97.41%   96.17%   -1.24%     
==========================================
  Files         164      167       +3     
  Lines        1470     1492      +22     
  Branches      361      373      +12     
==========================================
+ Hits         1432     1435       +3     
- Misses         34       50      +16     
- Partials        4        7       +3     
Components Coverage Δ
@modern-kit/react 93.01% <73.27%> (-2.20%) ⬇️
@modern-kit/utils 100.00% <100.00%> (ø)

@ssi02014
Copy link
Contributor Author

@Sangminnn 테스트 코드, 문서는 추후에 작업하겠습니다

* setLightMode(); // isDarkMode: false, isDarkModeOS: false
*
* setDarkMode(); // isDarkMode: true, isDarkModeOS: false
*/
Copy link
Collaborator

@Sangminnn Sangminnn Sep 27, 2024

Choose a reason for hiding this comment

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

전반적으로 너무 좋지만 두가지정도 고민인 포인트가 있는데요!

  1. useDarkMode라는 네이밍이다보니 반환해주는 setLightMode가 무언가 이질감을 주는것같습니다. darkmode에 대한 핸들링을 제공해주는 훅처럼 느껴지는데 lightMode로의 전환 핸들러를 제공해준다는 점이 사용성 자체는 괜찮아보이나 꼭 필요할지는 약간 고민이 되네요!

  2. 이 훅을 사용하는 유저의 입장을 생각해보면, 사용자의 system color mode에 따라 다른 color palette를 보여주려고 하는것이 일반적인 동작일것같은데요, 실제로 hooks을 사용할때 setter들의 역할도 너무 유용하지만 변경된 color scheme의 값 자체가 궁금한 경우도 있을것같습니다!
    물론 현재 isDarkMode라는 상태값을 반환해주고있기때문에 이를 활용한다면 조건문을 통해 현 상태를 체크할 수 있지만 번거로운 코드가 추가되는듯 하여 darkMode를 제공하는 훅임에도 colorScheme을 반환값으로 같이 내려주는건 어떨까요 ?? 👀

하지만 위의 방식은 실제로 사용처에서 usePreferredColorScheme을 호출하면 알 수 있는 부분이기때문에 작성하셨을때의 의도에 따라 다를 수 있을것같습니다!

작성해주실때 현재 colorScheme은 해당 훅을 통해 접근이 가능하니, 현재 상태가 필요한 경우는 useDarkMode를 쓰고있더라도 위의 훅을 선언해서 같이 활용하는 방법을 구상하셨을지 궁금합니다! 그렇게 생각하셨었다면 굳이 필요하지 않을것같다는 생각입니다 😄

// useDarkMode가 currentColorScheme을 반환해주었다면
const { isDarkMode, currentColorScheme } = useDarkMode()

useLayoutEffect(() => {
  //  colorManager.changeThemeMode('DARK'); 와 같이 바꾸어야한다면 아래와 같이 한번에 변경 가능
  colorManager.changeThemeMode(currentColorScheme);

  // 만약 해당 상태가 없다면 별도의 분기처리가 필요했을 것으로 예상
  colorManager.changeThemeMode(isDarkMode ? 'dark' : 'light')
}, [currentColorScheme])

Copy link
Contributor Author

@ssi02014 ssi02014 Sep 27, 2024

Choose a reason for hiding this comment

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

@Sangminnn 의견 감사합니다 :)

useDarkMode라는 네이밍이다보니 반환해주는 setLightMode가 무언가 이질감을 주는것같습니다. darkmode에 대한 핸들링을 제공해주는 훅처럼 느껴지는데 lightMode로의 전환 핸들러를 제공해준다는 점이 사용성 자체는 괜찮아보이나 꼭 필요할지는 약간 고민이 되네요!

1번의 경우에는 네이버의 경우에도 라이트 모드, 다크 모드, 기기 설정(이것도 추가하면 좋겠네요 네이밍 고민 필요🤔) 을 각각 셋팅 할 수 있는 기능을 제공하고 있기 때문에 충분히 유용 할 수 있다고 생각했습니다.

스크린샷 2024-09-28 오전 1 33 17

이 훅을 사용하는 유저의 입장을 생각해보면, 사용자의 system color mode에 따라 다른 color palette를 보여주려고 하는것이 일반적인 동작일것같은데요, 실제로 hooks을 사용할때 setter들의 역할도 너무 유용하지만 변경된 color scheme의 값 자체가 궁금한 경우도 있을것같습니다!
물론 현재 isDarkMode라는 상태값을 반환해주고있기때문에 이를 활용한다면 조건문을 통해 현 상태를 체크할 수 있지만 번거로운 코드가 추가되는듯 하여 darkMode를 제공하는 훅임에도 colorScheme을 반환값으로 같이 내려주는건 어떨까요 ?? 👀

다른 레퍼런스를 보니까 보통 boolean 형태로 많이 활용하는 것을 보고 boolean 형태로 관리하는 방향을 가졌었고, 실제 말씀처럼 usePreferredColorScheme을 활용해서 해당 값을 가져 올 수 있기 때문에 제외하였습니다.
하지만, usePreferredColorScheme를 반복 호출하기 보다 colorScheme을 그냥 함께 반환하는 것도 큰 문제가 아니기 때문에 반환 데이터에 추가해도 좋을 것 같다는 생각입니다.

https://usehooks-ts.com/react-hook/use-dark-mode

Copy link
Contributor Author

@ssi02014 ssi02014 Sep 27, 2024

Choose a reason for hiding this comment

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

useDarkMode라는 네이밍을 기반으로 접근하다보니 어색한 부분이 있다면 useThemeMode? useColorMode ? useColocScheme? 와 같은 네이밍은 어떠실까요

useThemeMode는 아래와 같은 레퍼런스가 존재합니다.
https://reactnativeelements.com/docs/customization/themeprovider#usethememode

useColocScheme 이라는 네이밍도 활용되네요 위 아래 둘다 reactNative이긴한데 ㅋㅋㅋ
https://reactnative.dev/docs/usecolorscheme

useColoeMode는 chakra와 같은 곳에서 사용하네요 😂
https://v2.chakra-ui.com/docs/styled-system/color-mode#usecolormode

각 네이밍에 대한 레퍼런스가 이미 존재하기 때문에 네이밍은 뭐가 됐든 상관 없겠네요

Copy link
Contributor Author

@ssi02014 ssi02014 Sep 27, 2024

Choose a reason for hiding this comment

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

@Sangminnn 고민의 결과 제 의견의 결론은 아래와 같습니다!! 🤗 어떠실까요!

  1. 훅의 네이밍 변경 제안 -> useThemeMode or useColocScheme or useColoeMode
  2. 다크 모드, 라이트 모드, OS 기기 설정 , toggle 4가지의 셋업 함수 제공
  3. preferredColorScheme 값을 반환 데이터로 추가

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ssi02014 긍정적으로 의견 받아주셔서 감사합니다! 👍 저는 제안해주신 부분 너무 좋습니다!

제안해주신 3가지를 적용한다면 기존에 이질감이 느껴졌던 부분도 해소될 것으로 보이고, preferredColorScheme도 반환받을 수 있어 별도 훅을 호출하지 않고도 해당 값을 사용할 수 있어 사용성이 더 높아질 것으로 생각됩니다! 👍👍👍

@ssi02014
Copy link
Contributor Author

ssi02014 commented Sep 29, 2024

@Sangminnn
변경 및 추가 작업을 진행했으며, 작업 내용은 아래와 같습니다!

  1. useColorScheme으로 네이밍을 변경하였습니다.
  2. 추가 개선 사항으로 안드로이드 10 미만에서는 prefers-color-scheme을 지원하지 않는 이슈가 있습니다. 따라서, 기본 값으로 우선적으로 사용되는 defaultValue옵션을 추가하였습니다.
useColorScheme({
  defaultValue: userAgent.includes('{Dark Mode Property}') ? 'dark' : 'light', // 함수도 가능
}); 
  1. body class에 colorScheme을 저장 여부를 결정하는 shouldSetBodyClass 옵션을 추가하였습니다.
  1. ColorScheme이라는 훅 네이밍과 preferredColorScheme을 반환하기 때문에 boolean 형인 isDarkMode 반환이 아닌 그냥 colorScheme/preferredColorScheme을 반환하는 방향으로 수정하였습니다.
  2. set함수 setToggleMode, setDarkMode, setLightMode, setColorSchemeToPreferredMode 4가지 함수를 반환합니다.


  1. 추가 개선 사항으로 안드로이드 10 미만에서는 prefers-color-scheme을 지원하지 않는 이슈가 있습니다. 따라서, defaultValue옵션을 추가하였습니다.

2번의 경우 prefers-color-scheme을 지원하지 않는 경우도 있을 수 있으며, 외부에서 다크모드 관련 데이터를 전달할 수 도 있습니다. (예를 들오, 앱에서 웹뷰를 띄울 때 UserAgent로 관련 데이터 전달)도 있습니다. 이런 경우에 defaultValue를 활용하면 대응 할 수 있을 것으로 보입니다.

하지만 set 함수를 직접 호출하는 경우에는 명시적으로 colorSchem을 지정한 경우이기 때문에 storage에 해당 값을 저장하고, 이를 가져옵니다.

위와 같은 방식을 사용해서 외부에서 제공받는 colorScheme을 유지해 줄 수도 있고, 웹이 단독으로 사용될 때의 자체적인 컬러모드 정책도 편리하게 결정할 수 있게 됩니다.

// localStorage에 저장된 값을 컬러모드로 사용할 수 있습니다.
const colorMode = window.localStorage.getItem('color_mode');
window.document.body.classList.add(colorMode);

// 웹뷰로 사용되는 경우에 userAgent에 앱의 컬러모드 값을 전달받아 사용할 수도 있습니다. 
// 이 경우 웹뷰가 열릴 때 앱팀의 지원을 받아 변경된 UA값을 전달 해 줘야 합니다
const isDarkMode = window.navigator.userAgent.inclues('{isDark property}');
if(isDarkMode){
  window.document.body.classList.add('dark')
}

// 앞서 사용한 prefers-color-scheme 값을 확인 해 시스템의 컬러모드 초기값으로 사용할 수도 있습니다.
if(window.matchMedia('(prefers-color-scheme: dark)').matches){
   window.document.body.classList.add('dark')
}

ts-doc에도 관련 내용을 남겨놨기 때문에 참고부탁드립니다! 궁금한 사항이 있다면 말씀주시면 감사드립니다!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature 새로운 기능 추가 @modern-kit/react @modern-kit/react
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants