Conversation
fitzgen
left a comment
fitzgen
left a comment
There was a problem hiding this comment.
@sepiropht it helps me help you if you describe any issues you are having in a WIP PR, and which parts are still in progress, and if you need help/feedback or are just checkpointing some work. Without any context, I don't know if you want me to look at the code or just wait until it isn't marked WIP anymore, or if you are stuck and are banging your head against the wall, or...
Please communicate so everything goes smoothly :)
Overall, this is a good start, but needs a little bit of work before landing -- see inline comments below. Thanks!
src/node.rs
Outdated
| use std::mem; | ||
| use std::u32; | ||
|
|
||
|
|
There was a problem hiding this comment.
Nitpick: don't add an extra newline here.
| } | ||
| } | ||
|
|
||
| pub fn html_string<R>(component: &R) -> String |
There was a problem hiding this comment.
Nitpick: this function needs documentation and an example of using it.
src/render_context.rs
Outdated
| let cached_set = &RefCell::new(CachedSet::default()); | ||
| let bump = &Bump::new(); | ||
| let templates = &mut FxHashMap::default(); | ||
| RenderContext { |
There was a problem hiding this comment.
I assume this is failing to compile because it is trying to return something that contains a borrow of something from the stack frame we are returning from?
Since RenderContext contains a borrow, but we don't want callers to have to supply any parameters that we will borrow from, I think we should make this a callback-based API:
pub(crate) fn empty<F, T>(f: F) -> T
where
F: FnOnce(&mut RenderContext) -> T
{
// ...
f(&mut RenderContext { ... })
}
// and then usage would be like:
RenderContext::empty(|cx| {
// use cx here...
})Does that make sense?
src/render_context.rs
Outdated
| /// return an empty rendering context | ||
| pub_unstable_internal! { | ||
| pub(crate) fn empty() -> Self { | ||
| let cached_set = &RefCell::new(CachedSet::default()); |
There was a problem hiding this comment.
Nitpick: please indent code properly. Everything within a function body should be 4 spaces indented.
sepiropht
force-pushed
the
feature-html-string
branch
2 times, most recently
from
0677d4d to
f79215b
Compare
sepiropht
force-pushed
the
feature-html-string
branch
from
f79215b to
301ab1c
Compare
|
What's the status here? |
|
@theduke It is not finished yet, i'm still struggle a bit to add tests. |
What are you struggling with? Anything I can do to help smooth things out? |
3 participants
#82