Skip to content
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.

Sign up for GitHub

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

Jump to bottom

added onTap feature for chartJS #1690

Merged
merged 4 commits into from
Oct 23, 2024
Merged

added onTap feature for chartJS #1690

merged 4 commits into from
Oct 23, 2024

Conversation

TheNoumanDev
Copy link
Member

Ticket: #1685

Tested with the Android build and well with CanvasKit and HTML.

Example EDL:

- ChartJs:
    onTap: 
      executeCode:
        body: | 
          console.log("hi");
    config: |
      {
        "type": "pie",
        "data": {
          "labels": ["Red", "Blue", "Yellow"],
          "datasets": [{
            "data": [300, 50, 100],
            "backgroundColor": [
                "rgb(255, 99, 132)",
                "rgb(54, 162, 235)",
                "rgb(255, 205, 86)"
            ]
          }]
        }
      }

@TheNoumanDev TheNoumanDev mentioned this pull request Oct 14, 2024
Closed
@TheNoumanDev TheNoumanDev self-assigned this Oct 14, 2024
@TheNoumanDev TheNoumanDev linked an issue Oct 14, 2024 that may be closed by this pull request
Require onTap event on ChartJS widget #1685
Closed
kmahmood74
kmahmood74 reviewed Oct 14, 2024
View reviewed changes
modules/ensemble/lib/widget/visualization/chart_js.dart Outdated
// Add click event listener to the chart
document.getElementById("${widget.controller.chartId}").onclick = function(event) {
// Notify Flutter about the click event (no need to send data)
if (window.dispatchEvent) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since we are sending the event anyway, why not send data as well? It is important in general to send the data along with the event even if not needed for this use case

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, I have updated the available data in it.

modules/js_widget/lib/src/mobile/js_widget.dart Outdated
WebViewWidget(controller: controller),
WebViewWidget(
controller: controller,
gestureRecognizers: <Factory<OneSequenceGestureRecognizer>>{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

configure the gestureRecognizers only if a listener has been specified. You don't want to capture clicks if there is no listener

modules/js_widget/lib/src/web/js_widget.dart Outdated
@@ -100,7 +100,7 @@ class JsWidgetState extends State<JsWidget> {
void initState() {
if (widget.listener != null) {
addListener(widget.id, widget.listener!);
init(globalListener);
_setupFlutterWebCommunication();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the init method sets up the interop. How will it work without calling that method?

modules/js_widget/lib/src/web/js_widget.dart Outdated
@@ -114,6 +114,14 @@ class JsWidgetState extends State<JsWidget> {
});
super.initState();
}
// Setup Flutter web communication for JS interactions
void _setupFlutterWebCommunication() {
html.window.addEventListener('callFlutter', (event) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

who dispatches the callFlutter event? why not use the interop?

kmahmood74
kmahmood74 reviewed Oct 17, 2024
View reviewed changes
modules/js_widget/lib/src/mobile/js_widget.dart Outdated
gestureRecognizers: <Factory<OneSequenceGestureRecognizer>>{
Factory<OneSequenceGestureRecognizer>(
() => TapGestureRecognizer()
..onTapDown = (TapDownDetails details) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the point of this if you are not doing anything inside it? also as requested earlier, if you really need to add gesturerecognizer, add it only if there is an onTap that the developer has specified. In this case I think you don't even need this piece of code at all right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First i thought that, that as it is a webView, mobile gesture detector wont work unless we detect it and then pass it to JsBridge to communicate with web, but yeah read more about it and tested it without this, understood how it is working. Thanks for pointing out.

kmahmood74
kmahmood74 approved these changes Oct 22, 2024
View reviewed changes
Copy link
Collaborator

@kmahmood74 kmahmood74 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please test on web, ios and android. Also add schema, kitchen sink example and documentation as well. Thank you

@TheNoumanDev TheNoumanDev merged commit 29684b9 into main Oct 23, 2024
2 checks passed
TheNoumanDev added a commit that referenced this pull request Oct 29, 2024
@TheNoumanDev
Revert "Merge pull request #1690 from EnsembleUI/chartJS0x45"
5fd08f4 
This reverts commit 29684b9, reversing
changes made to 102ab7c.
TheNoumanDev added a commit that referenced this pull request Oct 29, 2024
@TheNoumanDev
Merge pull request #1708 from EnsembleUI/revertJsModuleCode
9ea0530 
Revert "Merge pull request #1690 from EnsembleUI/chartJS0x45"
@TheNoumanDev TheNoumanDev mentioned this pull request Oct 31, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kmahmood74 kmahmood74 kmahmood74 approved these changes

Assignees

@TheNoumanDev TheNoumanDev

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Require onTap event on ChartJS widget
2 participants
@TheNoumanDev @kmahmood74