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

Make ChartViewBase's _data optional. (Fixes #771) #772

Merged

Conversation

ospr
Copy link
Contributor

@ospr ospr commented Feb 23, 2016

Changed ChartViewBase's _data property declaration from _data! to _data? to allow for setting the chart's data back to nil. Updated the code base to property handle _data as an optional value.

@danielgindi
Copy link
Collaborator

Looks nice. With some code moving from alpha Swift to modern Swift also...

How thoroughly was this tested?

Changed ChartViewBase's _data property declaration from _data! to _data? to allow for setting the chart's data back to nil. Updated the code base to property handle _data as an optional value.
@ospr
Copy link
Contributor Author

ospr commented Feb 28, 2016

When I initially made this pull request I had verified that setting the BarLineChartViewBase object's data property to nil would cause it to behave properly and had verified that there were no other regressions by running through all of the charts in the demo project (along with toggling various options) and verifying that they behaved as expected.

However, I realized I forgot to test setting other chart type's data property to nil. I went through the demo project and added a "Toggle Data" option and then checked each chart type to verify that they behaved properly. I found a few minor bugs, fixed them, and squashed them back into this pull request. I made a separate request at #781 which includes the "Toggle Data" option if you wish to pull that in as well.

@danielgindi
Copy link
Collaborator

Great! Thanks!

danielgindi added a commit that referenced this pull request Feb 28, 2016
Make ChartViewBase's _data optional. (Fixes #771)
@danielgindi danielgindi merged commit 35a7cf3 into ChartsOrg:master Feb 28, 2016
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.

None yet

2 participants