-
Notifications
You must be signed in to change notification settings - Fork 0
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
Fix: Limit rules
's prop expression
#14
Conversation
|
Your org has enabled the Graphite merge queue for merging into mainAdd the label “merge-queue” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link. |
WalkthroughThe pull request introduces a new API called Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
Triggered from #14 by @black7375. Checking if we can fast forward Target branch ( commit 55ab16a3f934c059b4b4d7dd3953ebca290bb93c (HEAD -> main, origin/main, origin/HEAD)
Author: alstjr7375 <[email protected]>
Date: Wed Oct 2 17:44:44 2024 +0900
Fix: Typescript syntax, typos Pull request ( commit cf98bb8ae7f0126345562918987d7f0d5a9429e5 (pull_request/props-change)
Author: alstjr7375 <[email protected]>
Date: Tue Nov 26 21:39:24 2024 +0900
Fix: Limit `rules`'s prop expression It is possible to fast forward |
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: 0
🧹 Outside diff range and nitpick comments (3)
text/002-css-rules.md (3)
447-447
: Consider simplifying the props target syntax.The current array syntax for defining prop targets could be error-prone. Consider a more explicit object-based approach:
- props: ["color", "background", { size: { targets: ["padding", "margin"] }}] + props: { + color: "color", + background: "background", + size: { + targets: ["padding", "margin"] + } + }
Line range hint
447-471
: Consider specificity implications in compiled output.The current compilation strategy might lead to specificity issues when combining multiple variants. Consider implementing a more robust specificity management system.
Suggestions:
- Add a specificity calculation system
- Document the specificity hierarchy
- Consider implementing a specificity normalization step during compilation
Line range hint
1-1000
: Consider enhancing type safety and runtime performance.The current API design could benefit from additional type safety measures and runtime optimizations.
Recommendations:
- Add strict type checking for variant combinations
- Implement runtime caching for frequently used style combinations
- Consider adding compile-time validation for prop types
Example type enhancement:
type VariantProps<T> = { [K in keyof T]: T[K] extends Record<string, unknown> ? keyof T[K] : never; }; const button = rules<VariantProps<typeof variants>>({ // ... implementation });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
text/002-css-rules.md
(2 hunks)
🔇 Additional comments (1)
text/002-css-rules.md (1)
471-471
: Verify CSS variable collision handling.
The base value implementation needs to ensure it doesn't conflict with CSS variable declarations.
/fast-forward |
Triggered from #14 (comment) by @black7375. Trying to fast forward Target branch ( commit 55ab16a3f934c059b4b4d7dd3953ebca290bb93c (HEAD -> main, origin/main, origin/HEAD)
Author: alstjr7375 <[email protected]>
Date: Wed Oct 2 17:44:44 2024 +0900
Fix: Typescript syntax, typos Pull request ( commit cf98bb8ae7f0126345562918987d7f0d5a9429e5 (pull_request/props-change)
Author: alstjr7375 <[email protected]>
Date: Tue Nov 26 21:39:24 2024 +0900
Fix: Limit `rules`'s prop expression Fast forwarding $ git push origin cf98bb8ae7f0126345562918987d7f0d5a9429e5:main
To https://github.com/mincho-js/working-group.git
55ab16a..cf98bb8 cf98bb8ae7f0126345562918987d7f0d5a9429e5 -> main |
Type inference is difficult because I currently have so many object types.
I have proven that it is possible, but since autocompletion rarely works, we need to change it to as concise a form as possible.
It may be reintroduced in the future through performance improvements and updates to TypeScript.
Summary by CodeRabbit
New Features
rules()
API for dynamic and declarative style definitions.props
feature for better state management.Documentation
rules()
API and its features.