Updates to bindings for XYZ axes, Treemap, Pie, and Cell components #59

johnhaley81 · 2024-11-25T21:06:39Z

This does include 2 breaking changes:

XYZ Axes' tickSize is now a float instead of an int since ticks do not have to be whole numbers
In Pie the first arg which is the data that is coming into the event is nullable

I thought I'd group these breaking changes together so that a single release could be cut but I can split things apart if needed.

Summary of changes:

fix curried functions in event handlers for XAxis, YAxis, and ZAxis
change dataKey type from string to 'dataKey in XAxis, YAxis, and ZAxis
change tickSize type from int to float in XAxis, YAxis, and ZAxis
add transform and stroke props to XAxis, YAxis, and ZAxis
add style prop to ZAxis
add fillOpacity prop to Cell
add optional flags to Treemap props stroke, fill, isAnimationActive, animationDuration
wrap Pie event handlers data prop in Js.Nullable.t

johnhaley81 · 2024-11-26T00:44:46Z

Before I intro some breaking changes, I want to ensure that they are the best possible changes I can so I'm making this a draft as I iterate some more on these.

jchavarri

Looking good to me.

jchavarri · 2024-11-26T10:02:19Z

src/YAxis.re

+    ) =>
+  makeProps(
+    ~interval=?interval |> AxisInterval.encodeOpt,
+    ~onClick=?


This adds some extra runtime (example). But I guess we're fine with it? cc @davesnx

It already had the runtime on it with interval |> AxisInterval.encodeOpt here so I figured it was ok but I can rip these out.

I included them mostly to help upgrading since you'd have to rewrite all of the functions to be uncurried otherwise but since these have breaking changes in them anyways maybe that's ok?

jchavarri · 2024-11-26T10:02:44Z

src/ZAxis.re

@@ -10,7 +10,7 @@ external make:
    ~allowDuplicatedCategory: bool=?,
    ~axisLine: 'axisLine=?,
    ~className: string=?,
-    ~dataKey: string=?,
+    ~dataKey: 'dataKey=?,


What other types can be dataKey? Is it really anything? Is it worth documenting + including a link to official docs if they exist?

This is why I put this back into draft actually.

I've been hitting a lot of bindings here that are marked as anything but they really aren't so I've been replacing them with [@mel.unwrap] versions in my local copy and I'll update this PR when I have that in a good spot and confirmed they are all indeed working as intended.

johnhaley81 · 2024-11-27T23:43:09Z

@jchavarri I updated the PR with more correct bindings for the XYZ axes.

jchavarri

Nice! I tried to check some bindings against the official docs but they just say Function so 🤷

jchavarri · 2024-11-28T06:59:47Z

CHANGES.md

+- change `axisLine` type from `any` to `[ | `Bool(bool) | `Obj(Js.t({..}))]` in `XAxis`, `YAxis`, and `ZAxis`
+- change `dataKey` type from `string` to `[ | `Str(string) | `Int(int) | `Fn('dataObj => 'dataKey)]` in `XAxis`, `YAxis`, and `ZAxis`
+- change `label` type from `any` to `[ | `Str(string) | `Int(int) | `Float(float) | `Component(React.element) | `Obj(Js.t({..}))]` in `XAxis`, `YAxis`, and `ZAxis`
+- change `name` type from `string` to `[ | `Str(string) | `Int(int) | `Float(float)]` in `XAxis`, `YAxis`, and `ZAxis`
+- add `range` prop to `XAxis` and `YAxis`
+- change `tick` type from `any` to `[ | `Obj(Js.t({..})) | `Component(React.element) | `Bool(bool) | `Fn('tick => React.element)]` in `XAxis`, `YAxis`, and `ZAxis`
+- change `tickFormatter` type from `any` to `(. 'tick, int) => string` in `XAxis`, `YAxis`, and `ZAxis` and wrap it with a curried function
+- change `tickLine` type from `any` to `[ | `Bool(bool) | `Obj(Js.t({..}))]` in `XAxis`, `YAxis`, and `ZAxis`
+- change `tickSize` type from `float` to `[ | `Float(float) | `Int(int)]` in `XAxis`, `YAxis`, and `ZAxis`
+- fix curried functions in event handlers for `XAxis`, `YAxis`, and `ZAxis`
+- add `transform` and `stroke` props to `XAxis`, `YAxis`, and `ZAxis`
+- add `style` prop to `ZAxis`
+- add `fillOpacity` prop to `Cell`
+- add optional flags to `Treemap` props `stroke`, `fill`, `isAnimationActive`, `animationDuration`
+- wrap `Pie` event handlers data prop in `Js.Nullable.t`


Maybe we can add something along the lines of the PR description in one line ("Improve bindings for XYZ axes, Treemap, Pie and Cell components"), with a link to the PR for those interested on details.

I would also add "Breaking" before it.

example.

jchavarri · 2024-11-28T07:01:16Z

src/YAxis.re

+      [@mel.unwrap] [
+        | `Bool(bool)
+        | `Obj(Js.t({..}))


jchavarri · 2024-11-28T07:02:08Z

src/ZAxis.re

    ~unit: string=?,
    ~width: int=?,
-    ~yAxisId: string=?


Was this a typo?

https://recharts.org/en-US/api/ZAxis#zAxisId

It might have been a typo before but should be correct now

jchavarri · 2024-11-28T07:06:24Z

src/YAxis.re

+        | `Str(string)
+        | `Int(int)
+        | `Float(float)
+        | `Component(React.element)


I'd call this Element. (A component is what creates elements, aka the make function). Same for other usages in other modules.

Ah... I remember now that I named it that to be consistent with this. I feel like I should also update this as well.

liubko · 2024-11-28T10:00:55Z

src/XAxis.re

    ~domain: array('domain)=?,
    ~height: int=?,
    ~hide: bool=?,
    ~interval: AxisInterval.t=?,
-    ~label: 'label=?,
+    ~label:
+      [@mel.unwrap] [


Should we maybe replace some of those types in Utils with mel.unwrap too?

https://github.com/ahrefs/melange-recharts/blob/master/src/Utils.re#L135

I'd like to tackle this in another PR but I think this is a great suggestion.

PR open: #60

johnhaley81 · 2024-12-02T18:57:33Z

Nice! I tried to check some bindings against the official docs but they just say Function so 🤷

Yeah I had to do a lot of diving into the repo and runtime testing to figure these out 🫠

johnhaley81 · 2024-12-02T19:05:04Z

@jchavarri I left this marked as Unpublished in the CHANGES file since @liubko also raised a good point about making the types in Utils use [@mel.unwrap] as well and I think that having back-to-back breaking releases is not ideal.

johnhaley81 added 6 commits November 25, 2024 14:00

fix curried event handlers

d4cfcbd

uppdate dataKey, tickSize, transform, and stroke for XYZ axes

4553b69

add fillOpacity to Cell

d9c2068

add optional flags to optional Treemap props

0e07da3

wrap first prop in Pie event handlers with Js.Nullable.t

602f743

update CHANGES

a2359bb

johnhaley81 marked this pull request as draft November 26, 2024 00:44

jchavarri reviewed Nov 26, 2024

View reviewed changes

replace "any" types with more correct types for XYZ axes

db7dbb6

johnhaley81 marked this pull request as ready for review November 27, 2024 23:42

johnhaley81 requested a review from jchavarri November 27, 2024 23:42

jchavarri approved these changes Nov 28, 2024

View reviewed changes

liubko reviewed Nov 28, 2024

View reviewed changes

johnhaley81 added 2 commits December 2, 2024 11:59

update CHANGES to be less verbose

527b615

change Component to Element

edcbe31

johnhaley81 merged commit 46db576 into ahrefs:master Dec 2, 2024
2 checks passed

johnhaley81 deleted the update-bindings-2 branch December 2, 2024 19:07

johnhaley81 mentioned this pull request Dec 2, 2024

Remove types from Utils that can be represented by [@mel.unwrap] #60

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates to bindings for XYZ axes, Treemap, Pie, and Cell components #59

Updates to bindings for XYZ axes, Treemap, Pie, and Cell components #59

johnhaley81 commented Nov 25, 2024

johnhaley81 commented Nov 26, 2024

jchavarri left a comment

jchavarri Nov 26, 2024

johnhaley81 Nov 26, 2024

jchavarri Nov 26, 2024

johnhaley81 Nov 26, 2024

johnhaley81 commented Nov 27, 2024

jchavarri left a comment

jchavarri Nov 28, 2024

johnhaley81 Dec 2, 2024

jchavarri Nov 28, 2024

jchavarri Nov 28, 2024

johnhaley81 Dec 2, 2024

jchavarri Nov 28, 2024

johnhaley81 Dec 2, 2024

johnhaley81 Dec 2, 2024

liubko Nov 28, 2024

johnhaley81 Dec 2, 2024

johnhaley81 Dec 2, 2024

johnhaley81 commented Dec 2, 2024

johnhaley81 commented Dec 2, 2024

Updates to bindings for XYZ axes, Treemap, Pie, and Cell components #59

Updates to bindings for XYZ axes, Treemap, Pie, and Cell components #59

Conversation

johnhaley81 commented Nov 25, 2024

johnhaley81 commented Nov 26, 2024

jchavarri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnhaley81 commented Nov 27, 2024

jchavarri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnhaley81 commented Dec 2, 2024

johnhaley81 commented Dec 2, 2024