feat(mrml-core): add Fragment element
#437
Conversation
JadedBlueEyes
force-pushed
the
jade/fragment
branch
from
9a067be to
8baa50b
Compare
JadedBlueEyes
changed the title
Fragment elementFragment element
JadedBlueEyes
force-pushed
the
jade/fragment
branch
from
8baa50b to
8bb7a96
Compare
JadedBlueEyes
changed the title
Fragment elementFragment element
JadedBlueEyes
force-pushed
the
jade/fragment
branch
from
8bb7a96 to
bca0a0a
Compare
There was a problem hiding this comment.
👋 Thanks for your interest for mrml and for this first PR!
That's a good react-like idea and I see how it could be used.
First, considering it's not mjml compatible, if this gets merged at some point, it should be behind a feature flag, disabled by default.
Then, we have to find an alternative for this fn children(&self) -> Vec<_>. Rebuilding a Vec is not acceptable.
Finally, we have to put in place some integration tests with some component that are using siblings, raw_siblings, indexes, etc.
|
Moving your PR to draft considering the amount of commented code and the points mentioned. |
JadedBlueEyes
force-pushed
the
jade/fragment
branch
4 times, most recently
from
3fb0081 to
726ef4e
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #437 +/- ##
==========================================
- Coverage 92.89% 92.74% -0.15%
==========================================
Files 227 227
Lines 12203 13153 +950
==========================================
+ Hits 11336 12199 +863
- Misses 867 954 +87 ☔ View full report in Codecov by Sentry. |
| @@ -8,6 +9,22 @@ struct MjColumnExtra<'a> { | |||
| } | |||
|
|
|||
| impl<'root> Renderer<'root, MjColumn, MjColumnExtra<'root>> { | |||
| #[cfg(feature = "fragment")] | |||
| fn children_iter(&self) -> impl Iterator<Item = &MjBodyChild> { | |||
| fn folder<'root>(c: &'root MjBodyChild) -> Box<dyn Iterator<Item = &MjBodyChild> + 'root> { | |||
There was a problem hiding this comment.
Not sure about that but we might not have to go through a Box.
Also, I'm not a big fan of having the function declared like this, just to use it once after.
There was a problem hiding this comment.
Perhaps a trait would be better? The problem I had with that is that the default impl in a trait can't access fields on a struct, so I have to reimplement the function anyway
This component effectively just acts as a collection of children. However, when rendered, they behave as if directly under the parent element. This is useful for passing around reusable components, etc, in Rust code. In this commit, it is not actually possible to parse a Fragment. Instead, they must be constructed by Rust code. This is because of some non-configurable validation in xmlparser: https://github.com/RazrFalcon/xmlparser/blob/e54c2768f20814140c11e6c92f94ee74bfd54808/src/stream.rs#L432 Signed-off-by: Jade Ellis <[email protected]>
Signed-off-by: Jade Ellis <[email protected]>
Signed-off-by: Jade Ellis <[email protected]>
Signed-off-by: Jade Ellis <[email protected]>
JadedBlueEyes
force-pushed
the
jade/fragment
branch
from
2bf91ec to
908d1db
Compare
Signed-off-by: Jade Ellis <[email protected]>
JadedBlueEyes
force-pushed
the
jade/fragment
branch
from
908d1db to
e67713a
Compare
|
This has been stuck for a while now, closing it |
|
Hey @jdrouet - Sorry about the lack of communication here. I'm still interested in merging this, and I'd added improvements based on your suggestions, along with some tests. Are there any particular things you'd like to help move this along? |
This component effectively just acts as a collection of children. However, when rendered, they behave as if directly under the parent element. This is useful for passing around reusable components, etc, in Rust code.
Right now, it is not actually possible to parse a Fragment. Instead, they must be constructed by Rust code. This is because of some non-configurable validation in xmlparser:
https://github.com/RazrFalcon/xmlparser/blob/e54c2768f20814140c11e6c92f94ee74bfd54808/src/stream.rs#L432
I'm using this in mrmx, as it makes it quite a bit more pleasant to use.
This might have a slight performance impact, as
fn get_childrenallocates a Vector to help fold all children and nested Fragment children into one array. Before, this was directly calling.iter()onself.element.children. However, local benchmarking doesn't find any significant difference. I'm very much open to a different solution, though!