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.

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

Allow optional attribute values #712

Closed
wants to merge 1 commit into from
Closed

Conversation

Kromgart
Copy link
Contributor

@Kromgart Kromgart commented Oct 7, 2024

  • Added .attr_opt() method into builder-like API.

  • Allow optional attributes in view! macro, the syntax is:
    <attr-name>?=<optional_value>

  • If the attribute value is None, the attribute is removed, just like a bool attribute does with a false value.

This should fix #622 (and maybe #447?)

Example:

fn main() {
    sycamore::render(|| {
        type SigOptStr<'a> = ReadSignal<Option<Cow<'a, str>>>;

        let the_class = Some("extra-dark");
        let sig_counter: Signal<i32> = create_signal(0);

        let on_btn_click = move |_| {
            sig_counter.set(sig_counter.get() + 1);
        };

        let sig_style: SigOptStr = sig_counter.map(|i| {
            if i % 2 == 1 {
                Some("color: red;".into())
            } else {
                None
            }
        });

        let sig_main_id: SigOptStr = sig_counter.map(|i| {
            if i % 2 == 0 {
                Some(i.to_string().into())
            } else {
                None
            }
        });

        let sig_data_id: SigOptStr = sig_counter.map(|i| {
            if i % 3 == 0 {
                Some(i.to_string().into())
            } else {
                None
            }
        });

        view! {
            button(on:click=on_btn_click) { "Toggle" }
            p(
                // Quoted attribute, dynamic non-option value
                "counter"=sig_counter.to_string(),
                // Plain attribute, non-dynamic option value
                class?=the_class,
                // Plain attribute, dynamic option value
                id?=sig_main_id,
                // Dashed attribute, dynamic option value
                data-id?=sig_data_id,
                // Quoted attribute, dynamic option value
                "style"?=sig_style,
            ) {
                "Some text"
            }
        }
    });
}

If this approach is acceptable - let me know, I'll add some tests into sycamore-macro (but if some other tests would be needed - let me know where).

- add .attr_opt() method into builder API

- optional attributes in view! macro, the syntax is:
  `<attr-name>?=<optional_value>`

- if the attribute value is None, the attribute is removed, like
  a bool attribute with a `false` value
Copy link

codecov bot commented Oct 7, 2024

Codecov Report

Attention: Patch coverage is 38.98305% with 36 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@231c057). Learn more about missing BASE report.
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
packages/sycamore-web/src/node/ssr_node.rs 20.00% 12 Missing ⚠️
packages/sycamore-web/src/elements.rs 0.00% 8 Missing ⚠️
packages/sycamore-view-parser/src/parse.rs 68.42% 6 Missing ⚠️
packages/sycamore-macro/src/view.rs 63.63% 4 Missing ⚠️
packages/sycamore-reactive/src/maybe_dyn.rs 0.00% 3 Missing ⚠️
packages/sycamore-web/src/attributes.rs 0.00% 3 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #712   +/-   ##
=======================================
  Coverage        ?   63.50%           
=======================================
  Files           ?       52           
  Lines           ?     6823           
  Branches        ?        0           
=======================================
  Hits            ?     4333           
  Misses          ?     2490           
  Partials        ?        0           

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

@lukechu10
Copy link
Member

I was actually thinking of a bit of a different design where we can just keep the existing syntax but allow passing Option<T> as well to make it optional.

This would be done by changing the type of string attributes to something like Option<Cow<'static, str>> and automatically wrapping string values in Some(...).

We already have the machinery to do this with MaybeDyn.

@Kromgart
Copy link
Contributor Author

Kromgart commented Oct 7, 2024

Ah, ok then, no problem! :)

I was actually thinking of a bit of a different design where we can just keep the existing syntax but allow passing Option<T> as well to make it optional.

Yeah, that would look better and that was what I've tried initially, but got bogged down in untangling conflicting impls and tried this approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Optional attributes in view!
2 participants