mrml icon indicating copy to clipboard operation
mrml copied to clipboard

feat(mrml-core): add `Fragment` element

Open JadedBlueEyes opened this issue 1 year ago • 2 comments

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_children allocates a Vector to help fold all children and nested Fragment children into one array. Before, this was directly calling .iter() on self.element.children. However, local benchmarking doesn't find any significant difference. I'm very much open to a different solution, though!


     Running benches/basic.rs (C:\Users\jade\.cache\cargo-target\release\deps\basic-9ef9e838d0ba5c55.exe)
Benchmarking render button: Collecting 100 samples in estimated 5.0707 s 
                        time:   [16.882 µs 17.014 µs 17.161 µs]
                        change: [-3.1318% -0.5849% +1.9841%] (p = 0.66 > 0.05)
                        No change in performance detected.
Found 7 outliers among 100 measurements (7.00%)
  5 (5.00%) high mild
  2 (2.00%) high severe

     Running benches/template.rs (C:\Users\jade\.cache\cargo-target\release\deps\template-7a43bc935c493ad3.exe)
Benchmarking amario: Collecting 100 samples in estimated 5.4222 s (10k itera
                        time:   [525.70 µs 529.71 µs 534.01 µs]
                        change: [-3.9368% -1.2313% +1.1003%] (p = 0.37 > 0.05)
                        No change in performance detected.
Found 6 outliers among 100 measurements (6.00%)
  3 (3.00%) high mild
  3 (3.00%) high severe

JadedBlueEyes avatar Jun 19 '24 01:06 JadedBlueEyes

Moving your PR to draft considering the amount of commented code and the points mentioned.

jdrouet avatar Jun 25 '24 16:06 jdrouet

Codecov Report

Attention: Patch coverage is 74.18398% with 87 lines in your changes missing coverage. Please review.

Project coverage is 92.74%. Comparing base (5311e58) to head (726ef4e). Report is 61 commits behind head on main.

:exclamation: Current head 726ef4e differs from pull request most recent head e67713a

Please upload reports for the commit e67713a to get more accurate results.

Files Patch % Lines
packages/mrml-core/src/fragment/render.rs 32.20% 40 Missing :warning:
packages/mrml-core/src/fragment/parse.rs 0.00% 20 Missing :warning:
packages/mrml-core/src/fragment/print.rs 0.00% 15 Missing :warning:
packages/mrml-core/src/mj_body/children.rs 0.00% 2 Missing :warning:
packages/mrml-core/src/mj_accordion/render.rs 92.30% 1 Missing :warning:
packages/mrml-core/src/mj_carousel/render.rs 94.73% 1 Missing :warning:
packages/mrml-core/src/mj_head/mod.rs 94.44% 1 Missing :warning:
packages/mrml-core/src/mj_include/body/parse.rs 80.00% 1 Missing :warning:
packages/mrml-core/src/mj_include/body/render.rs 92.85% 1 Missing :warning:
packages/mrml-core/src/mj_include/head/parse.rs 83.33% 1 Missing :warning:
... and 4 more
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     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Jun 27 '24 15:06 codecov[bot]

This has been stuck for a while now, closing it

jdrouet avatar Oct 22 '24 15:10 jdrouet

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?

JadedBlueEyes avatar Oct 25 '24 12:10 JadedBlueEyes