-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
data-select event dispatched to parent #22
Conversation
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 |
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! |
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. |
@JoelEllis thanks for point me to that (I was previously unaware). |
@djole87 I've played around with your changes a bit, and it is a bit incomplete. There is no handler given to the To make this PR complete, please make the following changes:
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. |
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. Thanks again! |
@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 <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 |
I've created a separate PR (#29) that documents this. |
@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). |
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