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.

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

Feature: Add draggable spots functionality for LineChart #1428

Closed
wants to merge 152 commits into from

Conversation

G33kFreak
Copy link

@G33kFreak G33kFreak commented Aug 26, 2023

Hello!
First of all, thank you and all contributors for this awesome package, you rule 🔥

This PR closes #1420, closes #1247, I believe this package definitely needs this type of feature. So here is my approach to achieve it.

What's done

  • Added parameter isDraggable for the LineChartBarData.
  • Added dragSpotUpdateFinishedCallback for the LineTouchData which returns UpdatedDragSpotsData. It allows us to do some logic after dragging has been finished.
  • If there is any LineChartBarData with an isDraggable flag, we cache lineBarsData inside _LineChartState and update them if we need to, depending on FlTouchEvent.
  • Added didUpdateWidget for _LineChartState in order to react to updates in the widget's configuration changes.
  • Added a simple sample (preview of it is down below)

Preview

スクリーンショット 2023-01-25 12 13 31

Please check if this approach fits for you and let me know, I'll finish it up and add more docs, also feel free to suggest what's can be done better :)

@docs-page
Copy link

docs-page bot commented Aug 26, 2023

To view this pull requests documentation preview, visit the following URL:

docs.page/imanneo/fl_chart~1428

Documentation is deployed and generated using docs.page.

@dateolive
Copy link

Great, this is the effect I imagined, this is really great.

@G33kFreak
Copy link
Author

Great, this is the effect I imagined, this is really great.

Thanks a lot, I'm still waiting for updates from the maintainer, in meanwhile, you can use my fork and in case of any bugs feel free to report them here :)

@techouse
Copy link

@G33kFreak holy cow, this is awesome 🚀

@mehroze-zaidi
Copy link

mehroze-zaidi commented Sep 30, 2023

Great man, I really wanted that feature as this is my requirement. Now waiting for merge.

Bumps [actions/checkout](https://github.com/actions/checkout) from 2 to 4.
- [Release notes](https://github.com/actions/checkout/releases)
- [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md)
- [Commits](actions/checkout@v2...v4)

---
updated-dependencies:
- dependency-name: actions/checkout
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <[email protected]>
@imaNNeo
Copy link
Owner

imaNNeo commented Sep 30, 2023

Thank you for this great contribution!
I will add some comments now:

final spotIndex = touchResponse.lineBarSpots?.first.spotIndex;

if (spotIndex != null && barIndex != null) {
final isDraggable = widget.data.lineBarsData[barIndex].isDraggable;
Copy link
Owner

Choose a reason for hiding this comment

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

I'm a bit confused about the design, we have an isDraggable flag in our LineChartBarData class, also we have DragSpotUpdateFinishedCallback? dragSpotUpdateFinishedCallback; in our LineTouchData.
(I know we might have something similar in other places, and I admit that is a bad design)

But for now, what if we keep these two variables beside each other in the LineTouchData class?
What do you think?

Copy link
Owner

Choose a reason for hiding this comment

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

Also, we can add other kinds of callbacks for example dragSpotStartedCallback, dragSpotMovedCallback, and dragSpodFinishedCallback.

The other (perfect) solution is to touch the RenderBaseChart to add a DragGestureRecognizer to detect the gesture events using this predefined, then we can pass down the gesture events to the touchCallback event as a FlTouchEvent. This way, we can use the drag-drop feature in all kinds of charts with the same logic.

For example in LineChart, we can have a flag to do the spot drag-drop (that you already implemented) as an under-the-hood magic. Also, users can implement any other kind of interaction with the drag-drop feature.

What do you think?

Copy link
Author

@G33kFreak G33kFreak Oct 1, 2023

Choose a reason for hiding this comment

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

Thanks for the comments! I like the 2nd idea and will research and play a little bit with it.

Copy link
Author

@G33kFreak G33kFreak Oct 1, 2023

Choose a reason for hiding this comment

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

The other (perfect) solution is to touch the RenderBaseChart to add a DragGestureRecognizer to detect the gesture events using this predefined, then we can pass down the gesture events to the touchCallback event as a FlTouchEvent. This way, we can use the drag-drop feature in all kinds of charts with the same logic.

For example in LineChart, we can have a flag to do the spot drag-drop (that you already implemented) as an under-the-hood magic. Also, users can implement any other kind of interaction with the drag-drop feature.

What do you think?

So, after some checks about the additional gesture recognizer, I realized, that it's already done :D

See, the DragGestureRecognizer is an abstract class.

abstract class DragGestureRecognizer extends OneSequenceGestureRecognizer {
    // ...Its code here
}

However, the package already implements PanGestureRecognizer in the RenderBaseChart which extends DragGestureRecognizer and has everything we need to implement some kind of similar features in other charts as well.

Based on that, we already have the needed gesture recognizer and this PR just creates the wanted under-the-hood magic for the LineChart, if I got you right.

I'm a bit confused about the design, we have an isDraggable flag in our LineChartBarData class, also we have DragSpotUpdateFinishedCallback? dragSpotUpdateFinishedCallback; in our LineTouchData. (I know we might have something similar in other places, and I admit that is a bad design)

But for now, what if we keep these two variables beside each other in the LineTouchData class? What do you think?

To be honest, I don't really know what would be better here. My intention was to allow users to implement a draggable feature for particular LineChartBarData. For example, we can have a few bars, but only one of them is going to be draggable. It's similar to isCurved flag to me.

lineBarsData: [
    LineChartBarData(
        spots: spots1,
        isCurved: true,
        isDraggable: true,
    ),
    LineChartBarData(
        spots: spots2,
        isCurved: true,
        isDraggable: false,
        color: Colors.red,
    ),
],

So that's why I put this flag there.

Also, we can add other kinds of callbacks for example dragSpotStartedCallback, dragSpotMovedCallback, and dragSpodFinishedCallback.

Definitely! Added it in the commit f42ba99.
Anyway, we have to pay users' attention to be careful with using setState in the dragSpotUpdateCallback. Would be good to have some notes in the docs :)

@G33kFreak G33kFreak requested a review from imaNNeo October 1, 2023 12:46
imaNNeo and others added 19 commits March 29, 2024 01:45
# Conflicts:
#	CHANGELOG.md
#	test/chart/bar_chart/bar_chart_painter_test.mocks.dart
#	test/chart/bar_chart/bar_chart_renderer_test.mocks.dart
#	test/chart/line_chart/line_chart_painter_test.mocks.dart
#	test/chart/line_chart/line_chart_renderer_test.mocks.dart
#	test/chart/pie_chart/pie_chart_painter_test.mocks.dart
#	test/chart/pie_chart/pie_chart_renderer_test.mocks.dart
#	test/chart/radar_chart/radar_chart_painter_test.mocks.dart
#	test/chart/radar_chart/radar_chart_renderer_test.mocks.dart
#	test/chart/scatter_chart/scatter_chart_painter_test.mocks.dart
#	test/chart/scatter_chart/scatter_chart_renderer_test.mocks.dart
#	test/utils/canvas_wrapper_test.mocks.dart
#	test/utils/utils_test.mocks.dart
@G33kFreak G33kFreak marked this pull request as draft March 29, 2024 15:37
@G33kFreak G33kFreak marked this pull request as ready for review March 29, 2024 21:42
@G33kFreak
Copy link
Author

G33kFreak commented Mar 29, 2024

Well, it's been a while since I created this PR (shame on me, really sorry for everyone here who has been waiting for it), however, rebased it for you so now you can take a look, I'll try my best to finish it this time :)

@imaNNeo
Copy link
Owner

imaNNeo commented May 9, 2024

Hi, it seems there is a conflict on CHANGELOG.md. You don't need to update the CHANGELOG.
Also, there are 152 commits on your branch. Are you sure you want to keep them all?

@imaNNeo
Copy link
Owner

imaNNeo commented Oct 20, 2024

Closing, because it had no activity for a while

@imaNNeo imaNNeo closed this Oct 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet