-
Notifications
You must be signed in to change notification settings - Fork 43
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: new notification #473
Conversation
要不要用一些类似 https://jotai.org/ 的全局状态管理工具?不然 Context 越加越多 |
}; | ||
|
||
export function useNotify(): NoticeContextType { | ||
return React.useContext(NoticeContext) ?? { noticeCount: 0 }; |
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.
React.useContext(NoticeContext)
在没有provider的情况下是 null,所以会导致一些测试出错。
不过我感觉这里也不用具体给每个失败的测试都加上provider,就直接加了个fallback。
问问 @bangumi/frontend-collaborators |
这个pr功能上ok了,大家看看代码写的有没有啥问题👀 |
发现一个问题,在首页和通知页面 https://pr-473-sites--bangumi-next.netlify.app/ 的时候不会在标题显示通知数量,但是在小组页面则显示了通知数量。 https://pr-473-sites--bangumi-next.netlify.app/group/sandbox 是因为这几个页面没有Helmet吗? 不知道首页的Helmet应该加在什么地方? |
首页还没做吧,所以没地方加( |
那首页的留到后面再解决吧 这个PR的代码问题交给PR作者了,溜( |
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.
功能ok
加在 GlobalLayout 里? |
后面做首页的时候再说吧 |
Preview Deployment
|
follow up #239
ref #236