-
Notifications
You must be signed in to change notification settings - Fork 153
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
Builder API #269
Builder API #269
Conversation
@lukechu10 All tests should pass. I went through the logs and adjusted some code to run on rustc v1.53, but it seems the tests did not re-run. Not sure why. Also, API is stable enough for an initial audit. I've migrated our project to use the API as it currently stands and have no issues. Parts that remain untested are the |
Wow thanks for all the work. I'll try to get to this soon. |
Would you like to try to aim this feature for |
Yes. That sounds good. But no pressure though, that will take some time. |
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.
Thank you for all the work you put in and sorry for taking so long to review it :)
This looks really great!
/// Binds `sub` to the `value` property of the node. | ||
/// | ||
/// Note that this is the same as calling `.dyn_prop("value", sub)`. | ||
/// | ||
/// # Example | ||
/// ``` | ||
/// # use sycamore::prelude::*; | ||
/// | ||
/// # fn _test<G: GenericNode>() -> Template<G> { | ||
/// let value = Signal::new(String::new()); | ||
/// | ||
/// input() | ||
/// .bind_value(value) | ||
/// .build() | ||
/// # } | ||
/// ``` | ||
pub fn bind_value(&self, sub: Signal<String>) -> &Self { | ||
let sub_handle = create_memo(cloned!((sub) => move || { | ||
Some((*sub.get()).clone()) |
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.
(This comment applies to both the bind_value
and bind_checked
function. I couldn't select the whole thing on GitHub)
2 things:
- This should probably belong inside the html builder since it is html specific.
- Instead of having
bind_value
andbind_checked
, maybe a more genericbind_str
method that accepts anevent_name: &str
andsub: Signal<String>
and equivalentbind_bool
method.
event_name
would thus bevalue
orchecked
.
No problem! I'm going to be mostly MIA the next couple weeks due to traveling. But I'll try to work on this whenever I get a chance.
What I was thinking was eventually making this a public macro which users could use for extending the builder API to use custom typed tags, which would be very useful for quickly adding support for something like Svelte Native, along with web components UI frameworks. Also, since this is a very simple macro, it didn't merit, in my opinion, dropping to a full procedural macro.
huh...I remember fixing this...
Agreed.
I do like
Sure, we can remove these, though I can imagine some workflows, especially with dynamic tables or virtual lists where this kind of optimization could be useful. Perhaps mark the function as unsafe? (though this might break the builder flow) If not, I'm fine with removing it. |
No worries!
What I meant was generate_tag_functions! { enum HtmlTag { ... } } instead of #[apply(generate_tag_functions)]
enum HtmlTag { ... } because they are equivalent.
We might be able to do something about this with type-states but I think that belongs in another PR. |
If you don't mind, I can merge this now and fix the issues I mentioned in a later PR to not make this one too long. |
Go right ahead!
…On Thu, Oct 21, 2021, 18:45 Luke Chu ***@***.***> wrote:
*EXTERNAL EMAIL : *Exercise caution when responding, opening links,
or opening attachments.
If you don't mind, I can merge this now and fix the issues I mentioned in
a later PR to not make this one too long.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#269 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AM6Z663RXLMPDMXNK3I7IBLUICJY7ANCNFSM5E4I26WQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
I have not fully tested, nor do I think the current iteration has the right aesthetics, but I believe it's in the right direction! This is an implementation attempt for the builder API as mentioned in #268.