anp / moxie

lightweight platform-agnostic tools for declarative UI
https://moxie.rs
Apache License 2.0
828 stars 27 forks source link

Dom builder ergonomics #256

Closed simon-bourne closed 3 years ago

simon-bourne commented 3 years ago

This improve the DOM build ergonomics. See the new dom_builder example for details.

codecov[bot] commented 3 years ago

Codecov Report

Merging #256 (0367309) into main (86fe7f9) will decrease coverage by 8.53%. The diff coverage is 35.29%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #256      +/-   ##
==========================================
- Coverage   50.48%   41.94%   -8.54%     
==========================================
  Files          47       48       +1     
  Lines        8326     8334       +8     
==========================================
- Hits         4203     3496     -707     
- Misses       4123     4838     +715     
Impacted Files Coverage Δ
dom/src/lib.rs 10.00% <ø> (ø)
dom/src/text.rs 18.60% <0.00%> (-0.91%) :arrow_down:
dom/tests/custom_component.rs 3.22% <0.00%> (ø)
mox/tests/derive_builder.rs 91.30% <ø> (-1.01%) :arrow_down:
dom/tests/dom_builder.rs 6.25% <6.25%> (ø)
dom/src/interfaces/node.rs 100.00% <100.00%> (ø)
dom/src/macros.rs 83.58% <100.00%> (+0.24%) :arrow_up:
mox/src/lib.rs 92.30% <100.00%> (ø)
mox/tests/simple_builder.rs 100.00% <100.00%> (ø)
src/lib.rs 39.52% <0.00%> (-16.33%) :arrow_down:
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 86fe7f9...0367309. Read the comment docs.

anp commented 3 years ago

If you rebase onto main again you should be able to get meaningful results from CI.

simon-bourne commented 3 years ago

I applied all your suggestions in the comments, and all non-codecov checks are passing. The only actual reduction in code coverage I can see, is not including the "identity" impl NodeBuilder for Text. I felt that would be adding a test for the sake of coverage, but happy to add one if you think otherwise.

If you're happy with the coverage, and there's no other fixes, I think we're good to go.

Here's the code not covered by tests:

impl NodeBuilder for Text {
    type Target = Self;

    fn build(self) -> Self::Target {
        self
    }
}
anp commented 3 years ago

Awesome, thanks!