Skip to content
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.

Sign up for GitHub

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

tricky support for container styles #36

Closed
wants to merge 1 commit into from
Closed

tricky support for container styles #36

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 33 additions & 33 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions demo_respo/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,13 @@ respo = { path = "../respo/" }
js-sys = "0.3.69"
wasm-bindgen = "0.2.93"
console_error_panic_hook = "0.1.7"
uuid = { version = "1.10.0", features = ["v4", "js"] }
uuid = { version = "1.11.0", features = ["v4", "js"] }
serde = { version = "1.0.210", features = ["derive", "rc"] }
serde_json = "1.0.128"
respo_state_derive = { path = "../respo_state_derive" }

[dependencies.web-sys]
version = "0.3.70"
version = "0.3.76"
features = [
"console",
'Document',
Expand Down
13 changes: 11 additions & 2 deletions demo_respo/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use std::rc::Rc;

use inner_text::comp_inner_text;
use respo::css::respo_style;
use respo::{space, RespoAction};
use respo::{contained_styles, space, RespoAction};
use web_sys::Node;

use respo::ui::ui_global;
Expand Down Expand Up @@ -64,7 +64,7 @@ impl RespoApp for App {

Ok(
div()
.class(ui_global())
.class(ui_global() + " " + &style_container())
.style(respo_style().padding(12))
.children([
comp_counter(&states.pick("counter"), store.counted)?.to_node(),
Expand Down Expand Up @@ -97,3 +97,12 @@ fn main() {

app.render_loop().expect("app render");
}

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))
)
);
Comment on lines +101 to +108
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.

Suggested change
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))
)
);

10 changes: 5 additions & 5 deletions respo/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "respo"
version = "0.1.14"
version = "0.1.15"
edition = "2021"
description = "a tiny virtual DOM library migrated from ClojureScript"
license = "Apache-2.0"
Expand All @@ -12,14 +12,14 @@ readme = "README.md"
# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
js-sys = "0.3.69"
js-sys = "0.3.76"
wasm-bindgen = "0.2.93"
lazy_static = "1.5.0"
cirru_parser = "0.1.31"
# cirru_parser = { path = "/Users/chenyong/repo/cirru/parser.rs" }
rust-hsluv = "0.1.4"
serde = { version = "1.0.210", features = ["derive", "rc"] }
serde_json = "1.0.128"
serde = { version = "1.0.215", features = ["derive", "rc"] }
serde_json = "1.0.133"
# respo_state_derive = { path = "../respo_state_derive" }
respo_state_derive = "0.0.1"

Expand All @@ -28,7 +28,7 @@ respo_state_derive = "0.0.1"
crate-type = ["cdylib", "rlib"]

[dependencies.web-sys]
version = "0.3.70"
version = "0.3.76"
features = [
"console",
'Document',
Expand Down
115 changes: 115 additions & 0 deletions respo/src/node/css.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -736,6 +736,55 @@ where
name.into()
}
}
/// 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()
}
}
Comment on lines +739 to +787
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.


/// turns `src/a/b.rs` into `a_b`, (used inside macro)
pub fn css_name_from_path(p: &str) -> String {
Expand Down Expand Up @@ -814,3 +863,69 @@ macro_rules! static_styles {
$crate::static_style_seq!($a, &[$b, $c, $d, $e, $f]);
};
}

/// macro to create a public function of CSS rules with a slice at current file scope,
/// ```rust
/// respo::contained_style_seq!(the_name,
/// &[
/// ("&", respo::css::respo_style())
/// ]
/// );
/// ```
/// gets a function like:
/// ```ignore
/// pub fn the_name() -> String
/// ```
#[macro_export]
macro_rules! contained_style_seq {
($a:ident, $b:expr) => {
pub fn $a() -> String {
// let name = $crate::css_name_from_path(std::file!());
let name = $crate::css::css_name_from_path(std::module_path!());
$crate::css::declare_contained_style(format!("{}__{}", name, stringify!($a)), $b)
}
};
}

/// 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]);
};
}
Comment on lines +890 to +931
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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())
 /// );
 /// ```

Loading
Loading