gadfly361 / rid3

Reagent Interface to D3
MIT License
142 stars 6 forks source link

Pieces should support setting an id #2

Closed kaosko closed 6 years ago

kaosko commented 7 years ago

Thank you for this library, it makes it a little less painful to integrate d3 into reagent. Any chance you could add support for setting an element id for the pieces as well. When you are doing a lot interactive graphs, seems like the only reasonable way to target the (mouse) event handlers (on the overlays) are with an element selector. Would be nice if you could set the id in the map the same way as for the main container instead of: ` ;:id "focus"

:did-mount (fn [node _] (.attr node "id" "focus")) `

gadfly361 commented 7 years ago

@kaosko Thank you for the request! I would like to clarify where you'd like the ID to show up. Let's consider the example below:

(defn viz [ratom]
  [d3/viz
   {:id    "some-id"
   :ratom ratom
   :svg   {:did-mount (fn [node _]
              (-> node
                  (.attr "width" 200)
                  (.attr "height" 200)
                  (.style "background-color" "grey")))}
   :pieces
   [{:kind      :elem
        :class     "backround"
        :tag       "circle"
        ;; :id "id-on-g-tag" ... not currently supported
        :did-mount (fn [node _]
                   (-> node
                   ;; (.attr "id" "id-on-circle") ... supported
                   (.attr "cx" 100)
                   (.attr "cy" 100)
                   (.attr "r" 50)))}]
   }])

Would result in:

<div id="some-id">
  <svg width="200" height="200" style="background-color: grey;">
    <g class="rid3-main-container">
      <g class="backround"> ;; <-- id-on-g-tag would go here
    <circle cx="100" cy="100" r="50"> ;; <-- id-on-circle would go here
    </circle>
      </g>
    </g>
  </svg>
</div>

Are you looking for id-on-g-tag of id-on-circle?

kaosko commented 7 years ago

Thanks, I'd say on the actual specified element because the other way could surprise you. If you need it on the g container, you could always do:

{:kind  :container
   :class "focus"
   :id "focus-indicator"
:children
[{:kind ... }]}
gadfly361 commented 7 years ago

@kaosko Thanks for the clarification!

In that case, then adding the id in the did-mount function (i.e. "id-on-circle" in the example above) is the proper place.

The :class in the piece hash-map refers to the g tag that contains the element. So adding a sibling keyword :id that adds an id to the element (instead of the containing g tag) would be inconsistent. Also related, I have a strong preference to consolidate the modifications of the node inside the did-mount and did-update functions.

I really appreciate the suggestion, and I hope you continue to provide feedback :)

kaosko commented 7 years ago

Ah, true I agree. Perhaps adding an :id to the container g should anyway be supported together with :class.

gadfly361 commented 7 years ago

@kaosko That makes sense to me, I will reopen this issue for that purpose then :)

gadfly361 commented 6 years ago

@kaosko I have spent some time thinking about this issue, and I think, ultimately, I am going to choose not to support it.

The :class is currently placed on the g tag that contains the element (not the element itself). However, if we also added support for an :id, then it would break if the piece was an :elem-with-data ... because multiple items would get the same id.

Thanks again for the suggestion in this issue though! :)