Skip to content

Conversation

@tigerros
Copy link

@tigerros tigerros commented Dec 8, 2023

Closes #423

  • Introduces a way to style individual components with individual styles.
  • Made some properties public and cleaned some code.

Example:

render! {
    Button {
        theme: ButtonThemeWith {
            background: Some("blue".into()),
            font_theme: Some(FontThemeWith {
                color: Some("white".into()),
                ..Default::default()
            }),
            ..Default::default()
        },
        label { "Blue button" }
    }
}

Or with the theme_with macro:

render! {
    Button {
        theme: theme_with!(ButtonTheme {
            background: "blue".into(),
            font_theme: theme_with!(FontTheme {
                color: "white".into(),
            }),
        }),
        label { "Blue button" }
    }
}

This will only change the background and color of the button. Nothing else is affected. Subthemes are supported (e.g., FontThemeWith).

Internals

Example of defining a theme:

// use_theme.rs
define_theme! {
    %[component]
    pub Test<'a> {
        %[cows]
        cow_string: str,
        %[borrowed]
        borrowed_data: &'a Foo,
        %[owned]
        owned_data: Bar,
        %[subthemes],
        font_theme: FontTheme,
    }
}
  • The %[component] attribute will automatically generate docs that point to the component of the theme's name.

The following attributes are all field "separators". This means that you specify them only once per theme, and they will apply to all following fields until they encounter another separator. These must be in the same order, but you don't need to use all of them.

  • %[cows] indicates that the next fields should be wrapped in Cow<'static, _>. This is more specific than the others, but it's an extremely common use case, because strings should always be wrapped in Cow to allow constant theme templates, but also dynamic theme overrides.
  • %[borrowed] indicates that the next fields are borrowed data and should therefore be handled as such (this matters in the apply_optional function that the macro generates).
  • %[owned] indicates that the next fields are owned data and should therefore be handled as such (this matters in the apply_optional function that the macro generates).
  • %[subthemes] indicates that the next fields are other themes and should therefore be handled as such (this matters in the apply_optional function that the macro generates).

Example of using a theme (in freya-components crate):

use crate::theme::get_theme;
use freya_hooks::ButtonThemeWith;

#[derive(Props)]
pub struct ButtonProps {
    /// Theme override.
    #[props(optional)]
    pub theme: Option<ButtonThemeWith>,
    // ...
}

pub fn Button(cx: Scope<ButtonProps>) -> Element {
    // First argument is self explanatory, the second one is the `ThemeWith` struct, the third one is the name of the theme in the `Theme` struct
    let ButtonTheme { 
        padding,
        width,
        background,
        ..
    } = get_theme!(cx, &cx.props.theme, button);
    // ...
}

Notes

This can be merged, but there is a couple things left to do that are relevant:

  • Move layout styles to themes.
  • Eliminate repetition. This is the theme property definition, the let theme = ... statement, and the /// This inherits [FooTheme](freya_hooks::FooTheme) doc. It was extremely annoying adding all of these.
  • Make a macro for easier theme setting (get rid of ..Default::default and Some). Update: see theme_with macro.
  • Document new system in book.

- Move theme defs to component files. This is getting put on hold for now, since there's some cyclic dependency issues. However, in the future, it would indeed be good to move the themes somewhere else than freya-hooks.

@codecov
Copy link

codecov bot commented Dec 8, 2023

Codecov Report

Attention: 193 lines in your changes are missing coverage. Please review.

Comparison is base (708cdda) 52.47% compared to head (b61ff31) 52.63%.

❗ Current head b61ff31 differs from pull request most recent head 742c296. Consider uploading reports for the commit 742c296 to get more accurate results

Files Patch % Lines
crates/components/src/dropdown.rs 0.00% 37 Missing ⚠️
crates/components/src/table.rs 0.00% 24 Missing ⚠️
crates/components/src/body.rs 0.00% 15 Missing ⚠️
...components/src/scroll_views/virtual_scroll_view.rs 0.00% 14 Missing ⚠️
crates/components/src/button.rs 0.00% 13 Missing ⚠️
crates/components/src/scroll_views/scroll_view.rs 0.00% 13 Missing ⚠️
crates/hooks/src/theming/mod.rs 0.00% 13 Missing ⚠️
crates/components/src/scroll_views/scroll_bar.rs 0.00% 8 Missing ⚠️
crates/components/src/canvas.rs 0.00% 7 Missing ⚠️
crates/components/src/icons/arrow.rs 0.00% 7 Missing ⚠️
... and 13 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #425      +/-   ##
==========================================
+ Coverage   52.47%   52.63%   +0.15%     
==========================================
  Files         145      146       +1     
  Lines       12847    12808      -39     
==========================================
  Hits         6742     6742              
+ Misses       6105     6066      -39     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tigerros
Copy link
Author

tigerros commented Dec 9, 2023

There might be a bit of a problem with moving the themes to the component files... It's impossible because Theme is in the hooks crate, but it would depend on the themes in the components crate, but components depends on hooks. It's a cyclic dependency. Theme could also be relocated to components, but I don't know about all the theme hooks.

@marc2332
Copy link
Owner

marc2332 commented Dec 9, 2023

There might be a bit of a problem with moving the themes to the component files... It's impossible because Theme is in the hooks crate, but it would depend on the themes in the components crate, but components depends on hooks. It's a cyclic dependency. Theme could also be relocated to components, but I don't know about all the theme hooks.

Heh, that explains why the themes are in the hooks crate. I would leave them in the hooks crate for now, maybe they can be moved to their own crate at some point in the future

@marc2332 marc2332 added the enhancement 🔥 New feature or request label Dec 9, 2023
@tigerros tigerros marked this pull request as draft December 9, 2023 19:49
tigerros added 13 commits December 9, 2023 20:55
- Themes had duplicated docs in the form of "Theming properties for the `Foo` component." This commit introduces an attribute `%[component]` that will automatically generate those docs.

- The `Theme` suffix has been removed. Now to define a theme, it's enough to do just `pub Button {` and everything else is handled.

- The "attributes" have a new style: `%[attr_name]`. This replaces the old `..cows..` separators and such with `%[cows]`. It's impossible to make it the standard syntax of `#[attr_name]` because that is ambiguous. However, it is much closer
This changes the lifetime of `Cow`s from `'a` to `'static`. This removes lifetimes in certain places and is more efficient. Dynamic props still work, because a `String` can be converted to `Cow<'static, str>`. The more you know!
@tigerros
Copy link
Author

tigerros commented Dec 10, 2023

That was painful, but it's done! All layout props were moved to the themes. All examples have been updated. Please mark this as breaking though.

@tigerros
Copy link
Author

tigerros commented Dec 10, 2023

I added an example in examples/dynamic_theme.rs. Basically copy pasted counter but the count affects the
brightness of the buttons. This actually wouldn't have been possible previously! Well, it would, but it would be very verbose, unintuitive, and inefficient.

I also tried to update the book, but I probably didn't cover everything. As a side note, please check out #427. I tested the book to see any errors, but there was way too many of them and I couldn't be bothered to look through them to find out which ones are affected by this PR. #427 is basically a request to fix all the errors.

@tigerros tigerros marked this pull request as ready for review December 10, 2023 18:00
@tigerros
Copy link
Author

tigerros commented Dec 10, 2023

Ready for review, but it's breaking, because layout stuff has been moved to themes.

@marc2332 marc2332 self-requested a review December 10, 2023 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking enhancement 🔥 New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

enhancement: More flexible built-in component styling

2 participants