-
Notifications
You must be signed in to change notification settings - Fork 9
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): useColorScheme 신규 훅 추가 #489
Conversation
🦋 Changeset detectedLatest commit: 472f6f1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
d2a36d6
to
95c8ac3
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #489 +/- ##
==========================================
+ Coverage 97.41% 97.53% +0.11%
==========================================
Files 164 167 +3
Lines 1470 1499 +29
Branches 361 377 +16
==========================================
+ Hits 1432 1462 +30
+ Misses 34 33 -1
Partials 4 4
|
@Sangminnn 테스트 코드, 문서는 추후에 작업하겠습니다 |
95c8ac3
to
dbb1934
Compare
* setLightMode(); // isDarkMode: false, isDarkModeOS: false | ||
* | ||
* setDarkMode(); // isDarkMode: true, isDarkModeOS: false | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
전반적으로 너무 좋지만 두가지정도 고민인 포인트가 있는데요!
-
useDarkMode라는 네이밍이다보니 반환해주는 setLightMode가 무언가 이질감을 주는것같습니다. darkmode에 대한 핸들링을 제공해주는 훅처럼 느껴지는데 lightMode로의 전환 핸들러를 제공해준다는 점이 사용성 자체는 괜찮아보이나 꼭 필요할지는 약간 고민이 되네요!
-
이 훅을 사용하는 유저의 입장을 생각해보면, 사용자의 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])
There was a problem hiding this comment.
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번의 경우에는 네이버의 경우에도 라이트 모드
, 다크 모드
, 기기 설정(이것도 추가하면 좋겠네요 네이밍 고민 필요🤔)
을 각각 셋팅 할 수 있는 기능을 제공하고 있기 때문에 충분히 유용 할 수 있다고 생각했습니다.
이 훅을 사용하는 유저의 입장을 생각해보면, 사용자의 system color mode에 따라 다른 color palette를 보여주려고 하는것이 일반적인 동작일것같은데요, 실제로 hooks을 사용할때 setter들의 역할도 너무 유용하지만 변경된 color scheme의 값 자체가 궁금한 경우도 있을것같습니다!
물론 현재 isDarkMode라는 상태값을 반환해주고있기때문에 이를 활용한다면 조건문을 통해 현 상태를 체크할 수 있지만 번거로운 코드가 추가되는듯 하여 darkMode를 제공하는 훅임에도 colorScheme을 반환값으로 같이 내려주는건 어떨까요 ?? 👀
다른 레퍼런스를 보니까 보통 boolean 형태로 많이 활용하는 것을 보고 boolean 형태로 관리하는 방향을 가졌었고, 실제 말씀처럼 usePreferredColorScheme을 활용해서 해당 값을 가져 올 수 있기 때문에 제외하였습니다.
하지만, usePreferredColorScheme를 반복 호출하기 보다 colorScheme을 그냥 함께 반환하는 것도 큰 문제가 아니기 때문에 반환 데이터에 추가해도 좋을 것 같다는 생각입니다.
There was a problem hiding this comment.
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
각 네이밍에 대한 레퍼런스가 이미 존재하기 때문에 네이밍은 뭐가 됐든 상관 없겠네요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Sangminnn 고민의 결과 제 의견의 결론은 아래와 같습니다!! 🤗 어떠실까요!
- 훅의 네이밍 변경 제안 ->
useThemeMode
oruseColocScheme
oruseColoeMode
다크 모드
,라이트 모드
,OS 기기 설정
,toggle
4가지의 셋업 함수 제공- preferredColorScheme 값을 반환 데이터로 추가
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ssi02014 긍정적으로 의견 받아주셔서 감사합니다! 👍 저는 제안해주신 부분 너무 좋습니다!
제안해주신 3가지를 적용한다면 기존에 이질감이 느껴졌던 부분도 해소될 것으로 보이고, preferredColorScheme도 반환받을 수 있어 별도 훅을 호출하지 않고도 해당 값을 사용할 수 있어 사용성이 더 높아질 것으로 생각됩니다! 👍👍👍
45dd4f1
to
efd9260
Compare
@Sangminnn
useColorScheme({
defaultValue: userAgent.includes('{Dark Mode Property}') ? 'dark' : 'light', // 함수도 가능
});
2번의 경우 prefers-color-scheme을 지원하지 않는 경우도 있을 수 있으며, 외부에서 다크모드 관련 데이터를 전달할 수 도 있습니다. (예를 들오, 하지만 set 함수를 직접 호출하는 경우에는 명시적으로 colorSchem을 지정한 경우이기 때문에 위와 같은 방식을 사용해서 // 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에도 관련 내용을 남겨놨기 때문에 참고부탁드립니다! 궁금한 사항이 있다면 말씀주시면 감사드립니다! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prefer color scheme이 안드로이드 구버전에서 지원이 안되는 부분은 몰랐네요! 좋은 정보 감사합니다!!
수정하신 코드는 전반적으로 다 좋아보입니다! 너무 고생하셨습니다! 👍
Overview
feat(react): useColorScheme 신규 훅 추가
useColorScheme는 기본적으로는 prefers-color-scheme를 기반으로 사용자 운영체제에 설정된 화면 모드로 dark모드인지 light모드인지 판단합니다.
만약, 반환하는 set 함수들을 호출하게 되면 로컬 스토리지에 여부를 저장하며, 스토리지에 값이 존재한다면 해당 값을 기반으로 dark모드인지 ligth 모드인지 판단합니다.
PR Checklist
Contributing Guide