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 - ChartView Pan Ended Delegate Call #3612

Merged
merged 3 commits into from
Jan 22, 2019

Conversation

AntonTheDev
Copy link
Contributor

@AntonTheDev AntonTheDev commented Aug 27, 2018

Issue Link 🔗

Over the years, every I have come to implement this library, i have found that 1 feature for me has always been missing which is the ability to know when the user stopped panning across a chart to display values on the fly, and have it reset once the user lets go. I've yet to find a clean solution other than modifying the library, and I have added some callback logic to the ChartViewDelegate

Goals ⚽

To be able to install this from the master repo, and not always have to fork it to add this exact feature./

Implementation Details 🚧

Added and optional method to the ChartViewDelegate to tell the delegate that the pan gesture has reached a cancelled / ended state

public protocol ChartViewDelegate
{
     ...

    /// Called when a user stop highlighting values while panning
    @objc optional func chartViewDidEndPanning(_ chartView: ChartViewBase)
}

Testing Details 🔍

None, there are no specific tests. This should not effect the snapshotting at all, and has no effect on the existing codebase other than an optional callback.

@codecov-io
Copy link

codecov-io commented Aug 27, 2018

Codecov Report

Merging #3612 into master will increase coverage by 1.06%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3612      +/-   ##
==========================================
+ Coverage   29.94%   31.01%   +1.06%     
==========================================
  Files         118      114       -4     
  Lines       13793    10483    -3310     
==========================================
- Hits         4130     3251     -879     
+ Misses       9663     7232    -2431
Impacted Files Coverage Δ
Source/Charts/Charts/BarLineChartViewBase.swift 65.78% <ø> (+39.25%) ⬆️
Source/Charts/Charts/ChartViewBase.swift 19.51% <ø> (-2.08%) ⬇️
Source/Charts/Utils/Platform.swift 0% <0%> (-15.39%) ⬇️
Source/Charts/Charts/PieRadarChartViewBase.swift 5.63% <0%> (-14.07%) ⬇️
Source/Charts/Charts/CombinedChartView.swift 25% <0%> (-13.62%) ⬇️
...s/Data/Implementations/Standard/PieChartData.swift 18.91% <0%> (-13.29%) ⬇️
...s/Data/Implementations/Standard/ChartDataSet.swift 23.33% <0%> (-12.94%) ⬇️
Source/Charts/Utils/ChartUtils.swift 36.87% <0%> (-9.24%) ⬇️
Source/Charts/Utils/ViewPortHandler.swift 18.13% <0%> (-8.72%) ⬇️
Source/Charts/Utils/Platform+Accessibility.swift 31.57% <0%> (-5.46%) ⬇️
... and 63 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4fca1f7...33e8eb1. Read the comment docs.

@jjatie
Copy link
Collaborator

jjatie commented Sep 16, 2018

@liuxuan30 @petester42 This looks good to me!

@@ -25,6 +25,9 @@ public protocol ChartViewDelegate
/// - parameter highlight: The corresponding highlight object that contains information about the highlighted position such as dataSetIndex etc.
@objc optional func chartValueSelected(_ chartView: ChartViewBase, entry: ChartDataEntry, highlight: Highlight)

/// Called when a user stop highlighting values while panning
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe your documentation needs to be updated. Shouldn't this read something like Called when a user stops panning?

Copy link
Member

@liuxuan30 liuxuan30 left a comment

Choose a reason for hiding this comment

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

documentations need to be precise.

If many people want the delegate, we could merge.

@proped
Copy link

proped commented Oct 10, 2018

I need this. Could u merged it ?

Updated documentation in the code based on feedback
@AntonTheDev
Copy link
Contributor Author

I updated the comment accordingly in the code, hope this helps :)

@natan
Copy link

natan commented Oct 23, 2018

@liuxuan30 Any update on this?

@pmairoldi
Copy link
Collaborator

pmairoldi commented Oct 23, 2018

These changes seem fine to me. If we’re going add delegate calls for gesture recognizes we should think about if we need them for all states of the gestures. Do people care about when a pan starts, or when it is ongoing?

@liuxuan30
Copy link
Member

I'm open to merge.but as @petester42 said, if we add this, maybe we should add more?

@natan
Copy link

natan commented Oct 24, 2018

@liuxuan30 @AntonTheDev Great, thank you for moving this along!

@liuxuan30
Copy link
Member

shall we add at least didStartPanning to pair for this PR?

@AntonTheDev
Copy link
Contributor Author

AntonTheDev commented Oct 26, 2018

I believe the whole "didStartPanning" can be be assumed when "chartValueSelected" is called? Since you can't really start panning without making a callback. But once you take your finger off the screen, there was no way to tell that you did. The values do no unselect themselves

@anton-filimonov
Copy link

I believe it'd be useful to make the callbacks similar to what we have for scrollview. And actually call them in pretty much the same situations as ScrollView does.

@jjatie
Copy link
Collaborator

jjatie commented Oct 27, 2018

I second making the delegate behave and be named as closely to UIScrollViewDelegate as possible.

@liuxuan30
Copy link
Member

liuxuan30 commented Nov 1, 2018

@AntonTheDev I'm ok to merge this, but it's just strange to me that we had didEnd without didStart. Though you might be able to know it get started through chartValueSelected, I think it's still different events.

How about we append the didStartPanning in recognizer.state == NSUIGestureRecognizerState.began closure?

@Arkezis
Copy link

Arkezis commented Jan 17, 2019

Thanks for your awesome library ! It is really helping us :)
This PR would be awesome for our application, we are looking forward to merge it :)

@liuxuan30
Copy link
Member

@petester42 @jjatie how about this PR? shall we add a few more or just merge? I'm looking for a few PRs that can be merged soon and release a new version next week.

@liuxuan30 liuxuan30 merged commit f2795b9 into ChartsOrg:master Jan 22, 2019
@jlubeck
Copy link

jlubeck commented Nov 19, 2019

@AntonTheDev I'm ok to merge this, but it's just strange to me that we had didEnd without didStart. Though you might be able to know it get started through chartValueSelected, I think it's still different events.

How about we append the didStartPanning in recognizer.state == NSUIGestureRecognizerState.began closure?

Yes please! I'm running into this situation myself right now. chartValueSelected get called both on drag AND tap, but chartViewDidEndPanning only gets called when the user stops dragging. Definitely different use cases...

Specifically, when this chart gets embedded inside a UIScrollView, we want to stop the UIScrollView from dragging when we are dragging on the chart. So one would think we can stop the UIScrollView from dragging on chartValueSelected, and restart it on chartViewDidEndPanning

But the problem with this solution is that chartValueSelected is also called on a single tap on the chart. So when this happens, the UIScrollView stops dragging, but then we have no callback to restart it... And the user is stuck with an unscrollable UIScrollView until get actually drags on the Chart.

Hope this was clear. I'm about to leave for the day, but if I don't hear from anyone back I'll probably make a pull request myself tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants