data-select event dispatched to parent

[djole87]

The chart now dispatches data-select event when clicked to any of the bar chart candles so this one can be handled in the parent

[himynameisdave]

Hi, thank you for your pull request! :)

Not exactly sure I understand the value of this change, would mind elaborating a bit or providing some more specific use-cases? Is this something that is frappe-charts specific, or something more Svelte-specific? Could you point me to some documentation if possible?

[himynameisdave]

Ping @djole87 -- I'm keen to move forward and merge this, I was just hoping to get some more understanding so that I could add proper documentation for this feature. Thanks!

[JadedBlueEyes]

This links to the https://frappe.io/charts/docs/update_state/navigation feature. It could probably used for drill-downs, linking to other pages or similar stuff. I don't really see a reason not to have it.

[himynameisdave]

@JoelEllis thanks for point me to that (I was previously unaware).

[himynameisdave]

@djole87 I've played around with your changes a bit, and it is a bit incomplete. There is no handler given to the on:data-select, and this handler should be a prop so that consumers can hook into this event. In addition, this should be documented in the README.md.

To make this PR complete, please make the following changes:

1. Add the new prop under the rest, defaulting to a noop: export let onDataSelect = () => {};
2. Pass that prop to the root element: on:data-select={onDataSelect}
3. Document how this works in the README.

This is a great change, thanks for finding this. I would love to help get this merged as soon as possible, so let me know if you need a hand, as if you don't respond I may just pick up where you left off.

[JadedBlueEyes]

Hi again! Thanks for looking at this again!

I think the reason no handler is given is an attempt to use the Event Forwarding feature (demo). As far as I'm aware, that should work by default, even with this, although I haven't tested it. I do agree on adding it to the README though - it's easy to forget about things like that! If you want, I could open a separate PR to do that?

Thanks again!

[himynameisdave]

@JoelEllis I tested this event forwarding and confirms that it works as expected. It's a bit non-obvious though, so I'd still appreciate us documenting this in the README in order to call this PR complete. Please include that link to Svelte docs about event forwarding. Here's a rough example of how it works:

<script>
  import Chart from "svelte-frappe-charts";

  // ...  

  const onDataSelect = (event) => {
    console.log('Data select event fired!', event);
  }
</script>

<Chart
  data={data}
  on:data-select={onDataSelect}
  isNavigable
/>

Note that users must also provide the isNavigable prop in order for this to work (or else data points cannot be selected/the event never fires). Please include this in the documentation.

[JadedBlueEyes]

I've created a separate PR (#29) that documents this.

[himynameisdave]

@JoelEllis thank you for adding documentation. Normally I would request that documentation be added in the same PR as the feature, but that's not reasonable here as you are not the original PR author (sorry, I thought you were the same person as @djole87 when I wrote this comment).