Add a new macro for use in animating mouseover and other things

ryanechternacht commented 4 years ago

Goal

I was trying to add animations to my d3, using the this form found on the bl.ocks.org.

abbreviated:

svg.selectAll("circle")
     .data(dataset)
     .enter()
     .append("circle")
     .attr(circleAttrs)  // Get attributes from circleAttrs var
     .on("mouseover", handleMouseOver);

function handleMouseOver(d, i) {  // Add interactivity
    d3.select(this).attr({
        fill: "orange",
        r: radius * 2
    });
}

To do, I wrote the following cljs, which works fine.

{ ...               
  :did-mount
    (fn [node ratom] 
                 (rid3-> node
                              (.on "mouseover"
                               (fn [d i]
                                 (this-as this    ;; *
                                       (rid3-> (.select d3 this)  ;; *
                                               .transition
                                               (.duration 500)
                                               {:y (+ (y-scale d) 100))}))))))}

PR in use

I was hoping to simplify this a bit with a macro, and after a few attempts I wrote the macro (seen in code) which can be used like. The lines marked ;; * are the ones replaced by the macro

{ ...               
  :did-mount
    (fn [node ratom] 
                 (rid3-> node
                              (.on "mouseover"
                               (fn [d i]
                                 (rid3on->
                                  .transition
                                  (.duration 500)
                                  {:y  (+ (y-scale d) 100)})))))}

Discussion

1) This is my first attempt at writing a "real" macro, so any/all feedback is appreciated 2) This macro technically hides the this from the user, so if someone wanted that, they couldn't use the macro 3) I was hoping for the macro to cover more (ideally starting with the .on and going through the (.select d3 this#). This would hid the [d i] parameters that a user probably wants. We could include these in the macro, but it feels weird to me to have your macro spit variables into the local state. maybe that's not weird?

Tests

This should probably come with tests, but I wasn't sure how to run your current test suite -- let me know, and I'll gladly add some.

ryanechternacht commented 4 years ago

I added a second approach (based on comment 3 above), that does more boilerplate, but injects variables into the scope of the forms. Some people on the clojure slack suggested this may be a "timebomb" so be careful. I wanted to throw it out here to see what you thought.

Usage of the second form would be

{ ...               
  :did-mount
    (fn [node ratom] 
                 (rid3-> node
                              (rid3on->2 "mouseover" 
                                  ;;; 3 vars are now available for use: d, i, this
                                  .transition
                                  (.duration 500)
                                  {:y (+ (y-scale d) 100)})))}

It was suggested that the names for d, i, and this could be supplied to the macro itself and passed through, to prevent the weird name injection. something like (rid3on->2 "mouseover" this-name d-name i-name ...) and then d-name i-name and this-name would be available, so the user could control the names and prevent shadowing. I'm not sure how to do this (although I'm looking into it).

gadfly361 commented 4 years ago

@ryanechternacht I really appreciate that you took the time to open a PR, thank you!

Unfortunately, I think the syntactic sugar that this macro provides is not worth expanding the API. I would accept a PR in the README though if you'd like to add like a Tips & Tricks section (or w/e name you think is appropriate) and describe this macro and its use-case.

ryanechternacht commented 4 years ago

Yeah, makes sense. I was hoping this would come together more, but yeah, this isn't worth much as it stands now.

Is the best path to open a new PR for the README change?

gadfly361 commented 4 years ago

@ryanechternacht Good question, I would be happy with either of the following: 1) Use this PR, but rebase this branch to remove the initial commits, so only the README commit remains 2) Open up a new PR and reference this one in a comment

gadfly361 commented 4 years ago

@ryanechternacht I am going to close this. Please feel free to open up a new PR and reference this one if you'd like to add a blurb to the readme 👍

gadfly361 / rid3