activeadmin / arbre

An Object Oriented DOM Tree in Ruby
MIT License
762 stars 74 forks source link

Support nested attribute hashes rendered as hyphenated attributes #451

Closed Ikariusrb closed 1 year ago

Ikariusrb commented 1 year ago

Closes #450

Ikariusrb commented 1 year ago

Would you mind adding another context with the same test case pairing but testing that a data attribute as a full named key (with hyphens, not using nested hashes) works as expected?

I'm not quite certain what you're asking for here. You're asking for a test showing that the (exceedingly awkward) code that I (ab)used to get Arbre to emit a hyphenated attribute before still works? If that's what you're asking for, I can add a test easily, but IMO the code I used to do that was terrible and should never be encouraged, and adding a test to codify it as part of the developer contract would be encouragement in my eyes.

If that's not what you're asking for, I'm a bit lost, and require further clarification :)

javierjulio commented 1 year ago

@Ikariusrb no but I think we should still address the original issue. I'm good with the support here for nested hashes, so thank you! My understanding is that with your changes, it should resolve the original issue you had right? For example, this test case passes with your changes.

      it "should render the attributes to html" do
        tag.build id: "my_id", "data-action" => "action"
        expect(tag.to_s).to eq "<tag id=\"my_id\" data-action=\"action\"></tag>\n"
      end

I was thinking for completion it would be nice to have a set of tests like this in the pattern you set up, to further demonstrate that hyphenated keys rather than nested hashes also works as expected now. What do you think?

Ikariusrb commented 1 year ago

@javierjulio My PR doesn't change the ability to pass in hyphenated attributes directly at all. The problem is ruby itself doesn't support hyphenated symbols constructed with colon shorthand syntax, and Arbre expects to receive the attributes as keyword args, so you have to construct a hash, use .to_sym and rocket to get a hyphenated key, then double-splat it out to the keyword args to get it passed in as Arbre expects. I consider that workaround to've been mostly a hack.

Changing the call signature for the tag builders is something I'd rather not attempt to touch, as that's far more likely to break something than what I added. The approach would likely be to accept attributes as a hash. I believe ruby will gather keyword args and put them in a hash if there's a final empty positional arg in a method signature, but to make that change I'd want to review the tests with more care than I have time for to make sure all cases were covered, and even then I'd be worried someone would've done something fun and unique in the wild that would end up breaking.

Ikariusrb commented 1 year ago

@javierjulio - any update on the ability for this to be merged?

javierjulio commented 1 year ago

@Ikariusrb yes, I will get to this soon, thank you.

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.12 :tada:

Comparison is base (b09b813) 91.23% compared to head (faf4f99) 91.36%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #451 +/- ## ========================================== + Coverage 91.23% 91.36% +0.12% ========================================== Files 17 17 Lines 468 475 +7 ========================================== + Hits 427 434 +7 Misses 41 41 ``` | [Impacted Files](https://codecov.io/gh/activeadmin/arbre/pull/451?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=activeadmin) | Coverage Δ | | |---|---|---| | [lib/arbre/html/attributes.rb](https://codecov.io/gh/activeadmin/arbre/pull/451?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=activeadmin#diff-bGliL2FyYnJlL2h0bWwvYXR0cmlidXRlcy5yYg==) | `100.00% <100.00%> (ø)` | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=activeadmin). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=activeadmin)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

javierjulio commented 1 year ago

@Ikariusrb I rebased to get CI green again on the latest. I reviewed this and realize why this makes no changes to hyphenated attributes. I remember now why I asked though since I wanted to have a test case to demonstrate that it works as we didn't have a test for that. The reason why is that the arguments are forwarded and the last arg is a hash, so keyword arguments in this case should not matter. It wasn't dependent on your change. It's a related request to improve the tests.