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: add useTheme #2617

Merged
merged 16 commits into from
Sep 22, 2024
Merged

feat: add useTheme #2617

merged 16 commits into from
Sep 22, 2024

Conversation

ianzone
Copy link
Contributor

@ianzone ianzone commented Aug 1, 2024

[中文版模板 / Chinese template]

🤔 This is a ...

  • New feature
  • Bug fix
  • Site / documentation update
  • Demo update
  • TypeScript definition update
  • Bundle size optimization
  • Performance optimization
  • Enhancement feature
  • Internationalization
  • Refactoring
  • Code style optimization
  • Test Case
  • Branch merge
  • Other (about what?)

🔗 Related issue link

💡 Background and solution

📝 Changelog

Language Changelog
🇺🇸 English add useTheme hook
🇨🇳 Chinese 增加 useTheme 钩子

☑️ Self Check before Merge

⚠️ Please check all items below before review. ⚠️

  • Doc is updated/provided or not needed
  • Demo is updated/provided or not needed
  • TypeScript definition is updated/provided or not needed
  • Changelog is provided or not needed

@CLAassistant
Copy link

CLAassistant commented Aug 1, 2024

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

✅ liuyib
✅ ianzone
❌ crazylxr
You have signed the CLA already but the status is still pending? Let us recheck it.

@crazylxr
Copy link
Collaborator

crazylxr commented Aug 5, 2024

很有意思的一个 hook,麻烦提一个 RFC,我们先在 issue 里面详细讨论一下 api 的设计呢

@ianzone ianzone mentioned this pull request Aug 5, 2024
@ianzone ianzone requested a review from liuyib August 5, 2024 05:49
@ianzone
Copy link
Contributor Author

ianzone commented Aug 7, 2024

请问还有要改的吗?

@ianzone ianzone requested a review from liuyib August 7, 2024 06:06
@ianzone
Copy link
Contributor Author

ianzone commented Aug 8, 2024

需要再许可一次

@liuyib
Copy link
Collaborator

liuyib commented Aug 8, 2024

一般做动态主题时,会给 html 加上特定类名(如 .dark)或者特定属性(data-theme="dark") 等,来实现动态换肤,所以这个 hook 应该还需要加个回调参数 onChange,这样 theme 变动时,用户可以自己完成上述这些操作。@ianzone 你觉着呢?

代码我更新了一部分,你改之前 pull 一下

@ianzone
Copy link
Contributor Author

ianzone commented Aug 8, 2024

一般做动态主题时,会给 html 加上特定类名(如 .dark)或者特定属性(data-theme="dark") 等,来实现动态换肤,所以这个 hook 应该还需要加个回调参数 onChange,这样 theme 变动时,用户可以自己完成上述这些操作。@ianzone 你觉着呢?

代码我更新了一部分,你改之前 pull 一下

改好了

@ianzone
Copy link
Contributor Author

ianzone commented Aug 12, 2024

还有哪里需要改的吗?

@ianzone
Copy link
Contributor Author

ianzone commented Aug 12, 2024

还望及时处理,不然一直落后于主分支。

@liuyib
Copy link
Collaborator

liuyib commented Aug 12, 2024 via email

@crazylxr
Copy link
Collaborator

一般做动态主题时,会给 html 加上特定类名(如 .dark)或者特定属性(data-theme="dark") 等,来实现动态换肤,所以这个 hook 应该还需要加个回调参数 onChange,这样 theme 变动时,用户可以自己完成上述这些操作。@ianzone 你觉着呢?

代码我更新了一部分,你改之前 pull 一下

这个 onChange 是不是也可以不加呢,直接监听 theme 的变化就好了?

@crazylxr
Copy link
Collaborator

另外这个 hook 叫 useTheme 是不是不太合适呢,看到这个 hook 的名称无法很明确的知道这个 hook 干什么的,这个跟 darkMode 比较相关,是不是可以考虑 hook 的名字结合一下这个

@ianzone
Copy link
Contributor Author

ianzone commented Aug 22, 2024

一般做动态主题时,会给 html 加上特定类名(如 .dark)或者特定属性(data-theme="dark") 等,来实现动态换肤,所以这个 hook 应该还需要加个回调参数 onChange,这样 theme 变动时,用户可以自己完成上述这些操作。@ianzone 你觉着呢?
代码我更新了一部分,你改之前 pull 一下

这个 onChange 是不是也可以不加呢,直接监听 theme 的变化就好了?

赞同

@ianzone
Copy link
Contributor Author

ianzone commented Aug 22, 2024

另外这个 hook 叫 useTheme 是不是不太合适呢,看到这个 hook 的名称无法很明确的知道这个 hook 干什么的,这个跟 darkMode 比较相关,是不是可以考虑 hook 的名字结合一下这个

但是这个hooks包括了light, dark, system 三种mode,而且确实和主题相关,所以我认为命名合适

@ianzone
Copy link
Contributor Author

ianzone commented Sep 11, 2024

没问题的话可否合并一下?我需要用这个钩子。

Copy link
Collaborator

@crazylxr crazylxr left a comment

Choose a reason for hiding this comment

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

LGTM

@crazylxr crazylxr merged commit 5ddd8c2 into alibaba:master Sep 22, 2024
5 of 6 checks passed
@ianzone ianzone deleted the useTheme branch September 22, 2024 03:15
@Airkro
Copy link

Airkro commented Dec 5, 2024

这是一个新加的 hooks 却没有使用 minor 的版本号更新,

会对一部分人造成影响,实测中由于 v3.8.2 新加入的 useTheme() 使用了 window 对象,以往能在 nodejs 环境里跑通的单元测试都失败了

@crazylxr
Copy link
Collaborator

crazylxr commented Dec 5, 2024

这是一个新加的 hooks 却没有使用 minor 的版本号更新,会对一部分人造成影响

实测中由于 v3.8.2 新加入的 useTheme() 使用了 window 对象,以往能在 nodejs 环境里跑通的单元测试都失败了

对,很遗憾,昨天晚上再发布完写 release log 的时候才发现这个问题。

@Przeblysk
Copy link

#2689 看来已经发现问题了,现在会导致依赖 ahooks 的仓库都会出现这个问题

@crazylxr
Copy link
Collaborator

crazylxr commented Dec 5, 2024

这是一个新加的 hooks 却没有使用 minor 的版本号更新,

会对一部分人造成影响,实测中由于 v3.8.2 新加入的 useTheme() 使用了 window 对象,以往能在 nodejs 环境里跑通的单元测试都失败了

3.8.4 支持了 ssr ,解决了该问题

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.

6 participants