-
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
Improved backward compatibility #11
Conversation
|
WalkthroughThe pull request introduces significant changes across two documentation files. It establishes an alias for the Changes
Possibly related PRs
Poem
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 (
|
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. |
Triggered from #11 by @black7375. Checking if we can fast forward Target branch ( commit 5d8dd5d552dc59bf36dcb8ad8eb971494786db4b (HEAD -> main, origin/main, origin/HEAD)
Author: JeongJun Jo <[email protected]>
Date: Sun Aug 18 17:16:08 2024 +0900
Add: Default Variants, Composition, RulesVariants Pull request ( commit 8035fa6d1a84a315c84a8a98c9904a9e28b73b19 (pull_request/ve-rules)
Author: alstjr7375 <[email protected]>
Date: Tue Sep 24 17:28:52 2024 +0900
Fix: Improved backward compatibility 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 (5)
text/000-css-literals.md (2)
524-541
: LGTM! Clear example ofcssVariants()
usage.The addition of
cssVariants()
as an alias tostyleVariants()
improves backward compatibility, which aligns well with the PR objectives. The provided example clearly demonstrates both the definition and usage ofcssVariants()
, including proper typing for thevariant
prop.Consider adding a brief comment explaining that
cssVariants()
is an alias forstyleVariants()
to make it immediately clear to readers unfamiliar with Vanilla Extract. For example:// cssVariants() is an alias for styleVariants() const background = cssVariants({ primary: { background: "blue" }, secondary: { background: "aqua" } });
524-541
: Summary: Changes align well with PR objectivesThe introduction of
cssVariants()
as an alias forstyleVariants()
and the accompanying example effectively address the PR's goal of improving backward compatibility. This addition provides a clear path for users to transition to or continue using the API in a backward-compatible manner.As you continue to enhance backward compatibility, consider documenting any potential gotchas or migration paths for users transitioning from other CSS-in-JS solutions to this new API. This could be added to a separate migration guide or included in the RFC as a subsection on backward compatibility considerations.
text/002-css-rules.md (3)
377-400
: LGTM! Great addition of backward compatibility.The introduction of backward compatibility for the
base
field in Vanilla Extract's recipes is a valuable addition that aligns well with the PR objectives. The examples clearly demonstrate how to use thebase
field and explain its priority when used alongside other properties.Consider adding language identifiers to the code blocks for better syntax highlighting. For example:
-``` +```typescript const myRule = rules({ base { color: "blue", backgroundColor: "red" } });This small change will improve the readability and consistency of the documentation. <details> <summary>Tools</summary> <details> <summary>Markdownlint</summary><blockquote> 377-377: null Emphasis used instead of a heading (MD036, no-emphasis-as-heading) </blockquote></details> </details> --- Line range hint `1137-1146`: **Improved example for future functional `rules()`.** The correction of the `interface` typo and the updated example for functional `rules()` provide a clearer illustration of how this feature could work in the future. Consider adding a note to emphasize that this is a future possibility and not currently implemented. For example: ```markdown > Note: The following is an example of a potential future implementation and is not currently available in the API.
This addition would help prevent any confusion for readers who might mistake this for a current feature.
377-377
: Convert emphasis to proper heading for "Backward compatibility".The static analysis tool flagged the use of emphasis (**) for "Backward compatibility". For better document structure and accessibility, it's recommended to use a proper heading instead.
Please update the line as follows:
-**Backward compatibility** +### Backward compatibilityThis change will improve the document's structure and make it easier to navigate.
Tools
Markdownlint
377-377: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- text/000-css-literals.md (1 hunks)
- text/002-css-rules.md (4 hunks)
Additional context used
Markdownlint
text/002-css-rules.md
377-377: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
Additional comments not posted (2)
text/002-css-rules.md (2)
406-406
: Good catch on the typo correction.The change from
inteface
tointerface
is a necessary correction that improves the code quality and prevents potential confusion or errors.
689-692
: Improved structure for compound variants.The updated structure of the
compoundVariants
array is a positive change. Encapsulating thecolor
andsize
properties within avariants
object improves readability and aligns better with TypeScript syntax. This change also makes the code more consistent with other examples in the document.Also applies to: 724-727
/fast-forward |
Triggered from #11 (comment) by @black7375. Trying to fast forward Target branch ( commit 5d8dd5d552dc59bf36dcb8ad8eb971494786db4b (HEAD -> main, origin/main, origin/HEAD)
Author: JeongJun Jo <[email protected]>
Date: Sun Aug 18 17:16:08 2024 +0900
Add: Default Variants, Composition, RulesVariants Pull request ( commit 8035fa6d1a84a315c84a8a98c9904a9e28b73b19 (pull_request/ve-rules)
Author: alstjr7375 <[email protected]>
Date: Tue Sep 24 17:28:52 2024 +0900
Fix: Improved backward compatibility Fast forwarding $ git push origin 8035fa6d1a84a315c84a8a98c9904a9e28b73b19:main
To https://github.com/mincho-js/working-group.git
5d8dd5d..8035fa6 8035fa6d1a84a315c84a8a98c9904a9e28b73b19 -> main |
Description
To improve backward compatibility, we have made some changes to the RFC and made the cssVariants API clearer in the existing document.
Summary by CodeRabbit
New Features
cssVariants()
function with examples for defining background variants.base
field in Vanilla Extract's recipes, enhancing functionality in therules()
function.Bug Fixes
inteface
tointerface
in theMyRulesProps
interface for proper TypeScript syntax.Documentation
cssVariants()
andbase
field usage.