tricky support for container styles #36
Conversation
WalkthroughThe pull request includes updates to several Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant CSS
User->>App: Request View
App->>CSS: Apply Styles
CSS->>App: Return Styled View
App->>User: Render View
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
demo_respo/Cargo.toml(1 hunks)demo_respo/src/main.rs(3 hunks)respo/Cargo.toml(3 hunks)respo/src/node/css.rs(2 hunks)respo_state_derive/Cargo.toml(1 hunks)
🔇 Additional comments (6)
respo/src/node/css.rs (1)
867-888: LGTM! Macro implementation is clean and well-documented.
The macro follows established patterns and includes clear documentation with examples.
respo_state_derive/Cargo.toml (1)
Line range hint 14-19: LGTM: Safe dependency update
The update to syn 2.0.90 is a minor version bump that maintains the same feature set. This change is safe and appropriate for macro processing.
demo_respo/Cargo.toml (1)
20-21: 🧹 Nitpick (assertive)
Verify web-sys features for container query support
The web-sys update to 0.3.76 is appropriate, but we should verify that all necessary features for container query support are enabled.
Consider adding ResizeObserver to web-sys features if container size monitoring is needed for the container queries implementation.
Also applies to: 26-46
respo/Cargo.toml (3)
3-3: Version bump aligns with feature addition
The increment to version 0.1.15 is appropriate for adding container styles support.
Line range hint 31-53: Consistent web-sys feature set
The web-sys features list appears comprehensive for DOM manipulation needs. The version update matches demo_respo, maintaining consistency across the workspace.
15-23: Verify dependency version compatibility
The updates to js-sys, serde, and serde_json are minor version changes. However, let's verify there are no breaking changes or security issues.
✅ Verification successful
Let me check for security advisories for serde and serde_json as well, and verify their version consistency across the workspace.
Dependencies are safe and consistently used
The updated dependencies (js-sys 0.3.76, serde 1.0.215, and serde_json 1.0.133) have:
- No security vulnerabilities reported
- Consistent usage across the workspace where they appear
- Minor version updates that maintain backward compatibility
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any security advisories for the updated dependencies
gh api graphql -f query='
{
securityVulnerabilities(first: 10, ecosystem: RUST, package: "js-sys") {
nodes {
advisory {
summary
severity
}
vulnerableVersionRange
}
}
}
'
# Check if versions are consistent across workspace
rg "version = \"0\.3\.76\"" $(fd Cargo.toml)
Length of output: 468
Script:
#!/bin/bash
# Check for security advisories for serde and serde_json
gh api graphql -f query='
{
serde: securityVulnerabilities(first: 10, ecosystem: RUST, package: "serde") {
nodes {
advisory {
summary
severity
}
vulnerableVersionRange
}
}
serde_json: securityVulnerabilities(first: 10, ecosystem: RUST, package: "serde_json") {
nodes {
advisory {
summary
severity
}
vulnerableVersionRange
}
}
}
'
# Check version consistency for serde and serde_json
rg "serde = \{ version = \"1\.0\.215\"" $(fd Cargo.toml)
rg "serde_json = \"1\.0\.133\"" $(fd Cargo.toml)
Length of output: 864
| /// macro to create a public function of CSS rules(up to 5 tuples) at current file scope, | ||
| /// ```rust | ||
| /// respo::static_styles!(the_name, | ||
| /// ("&", respo::css::respo_style()) | ||
| /// ); | ||
| /// ``` | ||
| /// gets a function like: | ||
| /// ```ignore | ||
| /// pub fn the_name() -> String | ||
| /// ``` | ||
| /// if you have more styles to specify, use `static_style_seq!` instead. | ||
| #[macro_export] | ||
| macro_rules! contained_styles { | ||
| ($a:ident, $b:expr) => { | ||
| $crate::contained_style_seq!($a, &[$b]); | ||
| }; | ||
| // to allow optional trailing comma | ||
| ($a:ident, $b:expr,) => { | ||
| $crate::contained_style_seq!($a, &[$b]); | ||
| }; | ||
| ($a:ident, $b:expr, $c:expr) => { | ||
| $crate::contained_style_seq!($a, &[$b, $c]); | ||
| }; | ||
| ($a:ident, $b:expr, $c:expr,) => { | ||
| $crate::contained_style_seq!($a, &[$b, $c]); | ||
| }; | ||
| ($a:ident, $b:expr, $c:expr, $d:expr) => { | ||
| $crate::contained_style_seq!($a, &[$b, $c, $d]); | ||
| }; | ||
| ($a:ident, $b:expr, $c:expr, $d:expr,) => { | ||
| $crate::contained_style_seq!($a, &[$b, $c, $d]); | ||
| }; | ||
| ($a:ident, $b:expr, $c:expr, $d:expr, $e:expr) => { | ||
| $crate::contained_style_seq!($a, &[$b, $c, $d, $e]); | ||
| }; | ||
| ($a:ident, $b:expr, $c:expr, $d:expr, $e:expr,) => { | ||
| $crate::contained_style_seq!($a, &[$b, $c, $d, $e]); | ||
| }; | ||
| ($a:ident, $b:expr, $c:expr, $d:expr, $e:expr, $f:expr) => { | ||
| $crate::contained_style_seq!($a, &[$b, $c, $d, $e, $f]); | ||
| }; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Fix macro name in documentation example.
The documentation example incorrectly references static_styles! instead of contained_styles!.
Apply this fix to the documentation:
/// ```rust
-/// respo::static_styles!(the_name,
+/// respo::contained_styles!(the_name,
/// ("&", respo::css::respo_style())
/// );
/// ```| /// does internal work inside the macro `contained_style!(name, &styles)`. | ||
| /// inserts CSS as `<style .. />` under `<head ... />` element | ||
| /// notice that the code only generats once and being cached as DOM states, with extra `<contained> { ... }` wrapper | ||
| /// | ||
| /// NOT working for dynamic styles that changes over time, use inline styles instead. | ||
| pub fn declare_contained_style<T, U>(name: T, rules: &[(Option<String>, U, RespoStyle)]) -> String | ||
| where | ||
| T: Into<String> + Clone, | ||
| U: Into<String> + Clone + Display, | ||
| { | ||
| let mut defined_styles = CLASS_NAME_IN_TAGS.write().expect("access styles"); | ||
| if defined_styles.contains(&name.to_owned().into()) { | ||
| name.into() | ||
| } else { | ||
| let window = web_sys::window().expect("window"); | ||
| let document = window.document().expect("load document"); | ||
| let head = document.head().expect("head"); | ||
| let style_tag = document.create_element("style").expect("create style tag"); | ||
| style_tag | ||
| .set_attribute("id", &format!("def__{}", name.to_owned().into())) | ||
| .expect("name tag"); | ||
|
|
||
| let mut styles = String::from(""); | ||
| for (contained, query, properties) in rules { | ||
| styles.push_str( | ||
| &query | ||
| .to_string() | ||
| .replace("$0", &format!(".{}", &name.to_owned().into())) | ||
| .replace('&', &format!(".{}", &name.to_owned().into())), | ||
| ); | ||
| styles.push_str(" {\n"); | ||
| styles.push_str(&properties.to_string()); | ||
| styles.push_str("}\n"); | ||
|
|
||
| if let Some(contained) = contained { | ||
| styles = format!("{} {{\n{}\n}}", contained, styles); | ||
| } | ||
| } | ||
|
|
||
| style_tag.dyn_ref::<Element>().expect("into element").set_inner_html(&styles); | ||
| head | ||
| .append_child(style_tag.dyn_ref::<Element>().expect("get element")) | ||
| .expect("add style"); | ||
|
|
||
| defined_styles.insert(name.to_owned().into()); | ||
|
|
||
| name.into() | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM! Consider enhancing error handling.
The implementation is clean and follows existing patterns well. The container query support is implemented correctly.
Consider using custom error types instead of expect() calls for better error handling. For example:
- let mut defined_styles = CLASS_NAME_IN_TAGS.write().expect("access styles");
+ let mut defined_styles = CLASS_NAME_IN_TAGS.write().map_err(|e|
+ format!("Failed to access styles: {}", e))?;Committable suggestion skipped: line range outside the PR's diff.
| contained_styles!( | ||
| style_container, | ||
| ( | ||
| Some("@media only screen and (max-width: 600px)".to_owned()), | ||
| "&", | ||
| respo_style().background_color(respo::css::CssColor::Hsl(0, 0, 95)) | ||
| ) | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM! Consider mobile-first approach.
The container styles implementation is correct, but consider using a mobile-first approach with min-width instead of max-width for better responsive design practices.
Consider this alternative approach:
contained_styles!(
style_container,
(
- Some("@media only screen and (max-width: 600px)".to_owned()),
+ Some("@media only screen and (min-width: 600px)".to_owned()),
"&",
- respo_style().background_color(respo::css::CssColor::Hsl(0, 0, 95))
+ respo_style().background_color(respo::css::CssColor::Hsl(0, 0, 100))
)
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| contained_styles!( | |
| style_container, | |
| ( | |
| Some("@media only screen and (max-width: 600px)".to_owned()), | |
| "&", | |
| respo_style().background_color(respo::css::CssColor::Hsl(0, 0, 95)) | |
| ) | |
| ); | |
| contained_styles!( | |
| style_container, | |
| ( | |
| Some("@media only screen and (min-width: 600px)".to_owned()), | |
| "&", | |
| respo_style().background_color(respo::css::CssColor::Hsl(0, 0, 100)) | |
| ) | |
| ); |
mainly for media query and https://developer.mozilla.org/en-US/docs/Web/CSS/@container
Summary by CodeRabbit
New Features
Bug Fixes
Documentation