-
Notifications
You must be signed in to change notification settings - Fork 77
feat: enhance layer configuration with composer support in StyleContext and useStyleRegister #224
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
base: master
Are you sure you want to change the base?
feat: enhance layer configuration with composer support in StyleContext and useStyleRegister #224
Conversation
📝 WalkthroughWalkthrough本次更改重构了样式层(layer)配置机制,引入了 Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant StyleProvider
participant useStyleRegister
participant parseStyle
App->>StyleProvider: 提供 layer 配置(含 composer)
StyleProvider->>useStyleRegister: 传递 layer(含 composer)
useStyleRegister->>parseStyle: 调用 parseStyle,传递 composer
parseStyle->>parseStyle: 利用 composer 组合 layer 依赖
parseStyle-->>useStyleRegister: 返回组合后的 CSS
useStyleRegister-->>App: 注入生成的样式
Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/StyleContext.tsxOops! Something went wrong! :( ESLint: 7.32.0 ESLint couldn't find the config "prettier" to extend from. Please check that the name of the config is correct. The config "prettier" was referenced from the config file in "/.eslintrc.js". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. src/hooks/useStyleRegister.tsxOops! Something went wrong! :( ESLint: 7.32.0 ESLint couldn't find the config "prettier" to extend from. Please check that the name of the config is correct. The config "prettier" was referenced from the config file in "/.eslintrc.js". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. tests/layer.spec.tsxOops! Something went wrong! :( ESLint: 7.32.0 ESLint couldn't find the config "prettier" to extend from. Please check that the name of the config is correct. The config "prettier" was referenced from the config file in "/.eslintrc.js". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
…xt and useStyleRegister
a9a6365
to
156b69e
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #224 +/- ##
==========================================
+ Coverage 94.87% 94.91% +0.03%
==========================================
Files 33 33
Lines 2889 2930 +41
Branches 456 463 +7
==========================================
+ Hits 2741 2781 +40
- Misses 148 149 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/hooks/useStyleRegister.tsx (1)
412-412
: 建议改进 enableLayer 的检测逻辑当前的检测方式虽然有效,但可读性较差。
建议使用更清晰的检测方式:
- const enableLayer = 'composer' in (ctxLayer || {}); + const enableLayer = ctxLayer?.composer !== undefined;
🛑 Comments failed to post (1)
src/StyleContext.tsx (1)
55-59:
⚠️ Potential issue类型命名冲突需要解决
LayerConfig
在这里定义,但在src/hooks/useStyleRegister.tsx
中已经存在一个同名但结构不同的接口。建议重命名以避免混淆。建议将此处的
LayerConfig
重命名为LayerContextConfig
或LayerComposerConfig
:-export type LayerConfig = { +export type LayerComposerConfig = { /** Define the hierarchical order here */ composer: LayerComposer; };同时更新相关引用:
- layer?: LayerConfig; + layer?: LayerComposerConfig;- layer: boolean | LayerConfig; + layer: boolean | LayerComposerConfig;🤖 Prompt for AI Agents
In src/StyleContext.tsx around lines 55 to 59, the type LayerConfig conflicts with a different LayerConfig defined in src/hooks/useStyleRegister.tsx. Rename this LayerConfig to LayerContextConfig or LayerComposerConfig to avoid confusion, and update all references to this type accordingly throughout the codebase.
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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/StyleContext.tsx (1)
134-134
: 需要添加测试覆盖静态分析工具指出第134行(
noop
分支)缺少测试覆盖。当layer
为false
时会执行此分支。需要为
layer: false
的情况添加测试用例。你希望我帮助生成相应的测试代码吗?🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 134-134: src/StyleContext.tsx#L134
Added line #L134 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/StyleContext.tsx
(2 hunks)src/hooks/useStyleRegister.tsx
(5 hunks)tests/layer.spec.tsx
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/layer.spec.tsx
- src/hooks/useStyleRegister.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/StyleContext.tsx (1)
src/hooks/useStyleRegister.tsx (1)
LayerConfig
(31-34)
🪛 GitHub Check: codecov/patch
src/StyleContext.tsx
[warning] 134-134: src/StyleContext.tsx#L134
Added line #L134 was not covered by tests
🔇 Additional comments (3)
src/StyleContext.tsx (3)
130-141
: 层级标准化逻辑正确且向后兼容层级标准化逻辑实现良好,提供了从布尔值到
LayerConfig
的平滑迁移路径。当layer
为true
时使用默认组合器,为false
时使用空操作函数,这确保了向后兼容性。🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 134-134: src/StyleContext.tsx#L134
Added line #L134 was not covered by tests
104-107
: 接口设计支持灵活的层级配置
StyleProviderProps
接口巧妙地扩展了StyleContextValue
并重新定义了layer
属性,允许用户传入布尔值或完整的LayerConfig
对象,这提供了良好的开发者体验。
93-94
: 🛠️ Refactor suggestion改进默认组合函数的实现
defaultLayerComposer
函数使用join()
方法但没有指定分隔符,这可能导致层级名称连接在一起而无法区分。建议明确指定分隔符:
-export const defaultLayerComposer: LayerComposer = (dependency) => - Array.from(dependency).join(); +export const defaultLayerComposer: LayerComposer = (dependency) => + Array.from(dependency).join(', ');Likely an incorrect or invalid review comment.
export type LayerComposer = (dependency: ReadonlySet<string>) => string; | ||
export type LayerConfig = { | ||
/** Define the hierarchical order here */ | ||
composer: LayerComposer; | ||
}; |
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.
💡 Verification agent
🧩 Analysis chain
类型命名冲突需要解决
在 src/hooks/useStyleRegister.tsx
中存在另一个同名的 LayerConfig
接口,但具有不同的属性结构(name: string; dependencies?: string[]
),这可能导致类型冲突和混淆。
建议重命名其中一个接口以避免冲突:
-export type LayerConfig = {
+export type StyleLayerConfig = {
/** Define the hierarchical order here */
composer: LayerComposer;
};
并相应更新所有引用。
🏁 Script executed:
#!/bin/bash
# 验证是否存在 LayerConfig 类型冲突
rg -n "LayerConfig" --type ts
Length of output: 372
修复:解决 LayerConfig
类型命名冲突
在 src/StyleContext.tsx
与 src/hooks/useStyleRegister.tsx
中都定义了同名的 LayerConfig
,虽然位于不同模块,但在引入或统一导出时极易混淆,建议重命名一侧以避免歧义。
请在 src/StyleContext.tsx
中将 LayerConfig
重命名为 StyleLayerConfig
并更新所有引用:
- src/StyleContext.tsx:56
- src/StyleContext.tsx:90
- src/StyleContext.tsx:106
示例修改:
--- a/src/StyleContext.tsx
+++ b/src/StyleContext.tsx
@@ -53,7 +53,7 @@ export type LayerComposer = (dependency: ReadonlySet<string>) => string;
-export type LayerConfig = {
+export type StyleLayerConfig = {
/** Define the hierarchical order here */
composer: LayerComposer;
};
@@ -87,7 +87,7 @@ export interface StyleProviderProps {
/** 启用层级模式时的配置 */
- layer?: LayerConfig;
+ layer?: StyleLayerConfig;
}
export const StyleProvider: React.FC<StyleProviderProps> = ({
@@ -103,7 +103,7 @@ export const StyleProvider: React.FC<StyleProviderProps> = ({
/** 如果传入布尔值,则使用默认 composer;若传入对象则按定义解析 */
- layer?: boolean | LayerConfig;
+ layer?: boolean | StyleLayerConfig;
children,
}) => {
完成重命名后,请确认相关导出和调用处(如统一导出的 index.ts
)已同步更新,确保无遗漏。
🤖 Prompt for AI Agents
In src/StyleContext.tsx around lines 55 to 59, rename the LayerConfig type to
StyleLayerConfig to avoid naming conflicts with another LayerConfig in
src/hooks/useStyleRegister.tsx. Update all references to LayerConfig within
src/StyleContext.tsx, including lines 56, 90, and 106, to use StyleLayerConfig.
Also, ensure that any exports or imports related to this type, such as in
index.ts, are updated accordingly to maintain consistency and prevent confusion.
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.
Pull Request Overview
This PR enhances the CSS layer configuration by introducing support for custom layer composition via a composer function. Key changes include:
- Adding a new test in tests/layer.spec.tsx to validate custom layer composer behavior.
- Enhancing the logic in useStyleRegister to support custom composer integration.
- Updating StyleContext to define a new LayerComposer type and standardize layer configuration normalization.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
tests/layer.spec.tsx | Added test case for custom layer composer functionality |
src/hooks/useStyleRegister.tsx | Updated layer handling and composer integration in style registration |
src/StyleContext.tsx | Introduced LayerComposer type and improved layer config standardization |
Comments suppressed due to low confidence (1)
src/hooks/useStyleRegister.tsx:456
- [nitpick] Avoid using non-null assertions (layer! and ctxLayer!) without explicit null checks; consider adding safeguards to ensure these values are defined before accessing the composer property, reducing the risk of runtime errors.
...{ ...layer!, composer: ctxLayer!.composer }...
@@ -402,9 +406,10 @@ export default function useStyleRegister( | |||
transformers, | |||
linters, | |||
cache, | |||
layer: enableLayer, | |||
layer: ctxLayer, | |||
} = React.useContext(StyleContext); | |||
const tokenKey = token._tokenKey as string; |
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.
[nitpick] Consider adding an inline comment to explain the rationale behind checking if 'composer' exists in ctxLayer to determine enableLayer. This will improve code clarity for future maintainers.
const tokenKey = token._tokenKey as string; | |
const tokenKey = token._tokenKey as string; | |
// Check if 'composer' exists in ctxLayer to determine if layer functionality is enabled. |
Copilot uses AI. Check for mistakes.
@@ -52,7 +52,13 @@ export function createCache() { | |||
|
|||
export type HashPriority = 'low' | 'high'; | |||
|
|||
export interface StyleContextProps { | |||
export type LayerComposer = (dependency: ReadonlySet<string>) => string; |
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.
太复杂了,我们修复的话应该是让用户最简单的心智去处理。理论上来说,他们只要知道自己的上游依赖是什么就行了,而不用关心整体如何聚合出来。
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.
嗯嗯 get
background: ant-design/pro-components#8955
Summary by CodeRabbit
新功能
测试