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

All Charts Icons Support Swift3 [Dub #629, #624, #1261] #1793

Merged
merged 9 commits into from
Feb 20, 2017

Conversation

abjurato
Copy link
Contributor

@abjurato abjurato commented Nov 6, 2016

Finally, implemented Icons (Nodal Image) feature for all charts and updated to Swift3.
6ff3141e-5050-11e6-92e4-a3ccef6b1eaa6be3a3b6-5050-11e6-9eb5-48cb1cbac3ac
I've chosen to store image in data parameter of ChartDataEntry to allow icons to be specified per-value (example of use: to sign personal records in running application). Dataset can specify if it will draw icons and customise offset from default placement. IconOffset is CGSize, but for radial Radar and Pie only height value matters — as distance from centre.

ChartBaseDataSet, IChartDataSet:

  1. drawIconsEnabled:Bool flag and isDrawIconsEnabled() accessor for it
  2. iconsOffset:CGSize which allows to influence at placement of icon on chart in relation to value's text (wether it is visible or not)

ChartUtils:

  1. drawImage(context:CGContext, image:NSUIImage, point:CGPoint, expectedSize:CGSize, offset:CGSize) - draws image in context

All xxxChartRenderers:

  1. code to draw icon (if data set's isDrawIconsEnabled and image is included to entry's data) in drawValues() method

BarChartDataEntry:

  1. initialiser for stacked bar with data parameter — one data for whole stack

Demos: included star image to project, for stacked bar, pie, line, candle stick, bubble, bar and horizontal bar added Toggle Icon button and star to be drawn.

@ghost
Copy link

ghost commented Nov 6, 2016

Okay, how do I fix these checks to be green? On my mac everything is built successfully for all platforms (iOS, OSX, tvOS)

@liuxuan30
Copy link
Member

liuxuan30 commented Nov 7, 2016

@AnatolyRosencrantz update the test cases? In setup add record = true from ##500
@petester42 could help

@ghost
Copy link

ghost commented Nov 7, 2016

I've already generated those test images, put them to appropriate Tests/ReferenceImages_64/ folders (which is the only place I've found other test images). By some reason FBSnapshotTestCase was saving them under /Library/Developer/Xcode/DerivedData by default, btw.

@anfriis
Copy link

anfriis commented Nov 9, 2016

Really looking forward to this! :)

@ghost
Copy link

ghost commented Nov 9, 2016

Guys, does somebody know where travis expects FBSnapshotTestCase's images to be? There are dozen of folders called "Tests" in project's directory

@pmairoldi
Copy link
Collaborator

You don't have to move anything anywhere. When you record the tests they images will be placed in the right place. The setting for the path is an environment variable in the test scheme.

@ghost ghost force-pushed the icons3 branch 4 times, most recently from 56a8712 to fbba515 Compare November 10, 2016 09:38
@farazhaider88
Copy link

any update on this ??

@ghost
Copy link

ghost commented Nov 10, 2016

@farazhaider88 fighting with Travis to accept pr

@codecov-io
Copy link

codecov-io commented Nov 10, 2016

Current coverage is 18.80% (diff: 28.11%)

Merging #1793 into master will increase coverage by 0.13%

@@             master      #1793   diff @@
==========================================
  Files           124        124          
  Lines         14083      14239   +156   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           2629       2677    +48   
- Misses        11454      11562   +108   
  Partials          0          0          

Powered by Codecov. Last update 5d9c81a...ccaa966

@liuxuan30
Copy link
Member

all pass! @danielgindi seems we can put this higher priority?

@anfriis
Copy link

anfriis commented Nov 18, 2016

I tried out the pull request in my project and I am having trouble aligning the icons in the center of the chart piece. I have tried some different values for the iconOffset but it doesn't help.

skaermbillede 2016-11-18 09 58 47

@ghost
Copy link

ghost commented Nov 19, 2016

@Anders123fr, fixed that incorrect misplacement. So in case of Pie chart iconOffset's x value stands for distance from center, so it should be 0 if you want icon's center to be placed where value's text is drawn by default. Could you check again and confirm if everything works as expected?

@anfriis
Copy link

anfriis commented Nov 21, 2016

Awesome, you just made my day!
It works perfectly, and i am able to adjust how close the icon should be to the center of the pie chart using the height of the iconOffset.
The screenshot shows a chart with iconOffset set to CGSize(width: 0, height: -10)
skaermbillede 2016-11-21 10 01 43
Thank you!

@danielgindi
Copy link
Collaborator

This is getting better, and it seems like people are enjoying it :-)
I hope to get sometime to take a closer look soon, merge it, and port to Android

@AntonioFM92
Copy link

Why can't I see the file changed when I download this project from master? thanks

@ghost
Copy link

ghost commented Nov 28, 2016

@AntonioFM92 cuz it was not merged to master yet

@MrMage
Copy link

MrMage commented Dec 5, 2016

This just saved me a ton of work (was going to implement this my self), thanks a ton!

One question, though: Is it possible in a pie chart view to render the icons below the label text so that the two don't overlap? As was said before, at the moment, changing the offset draws the labels further outside rather than below.

@ghost
Copy link

ghost commented Dec 5, 2016

@MrMage pushed a fix. Now iconsOffset for Pie and Radar charts represents (vertical offset; distance form center offset).

  • So if you want icon to be rendered below the value text label, you should use something like CGSize(width: 15, height: 0).
  • If you want icon to be rendered closer to center, you should use something like CGSize(width: 0, height: -15).
  • Sure, you can combine these two things like CGSize(15, -15) and icon will be 15 points below text label and 15 points closer to center. Looks strange, as for me, but legit.

@ghost
Copy link

ghost commented Dec 12, 2016

@danielgindi still not a time? :)

@sashalondon
Copy link

sashalondon commented Feb 5, 2017

Is this not going to be in the main release?

@liuxuan30
Copy link
Member

I believe they are busy :(

@danielgindi
Copy link
Collaborator

danielgindi commented Feb 20, 2017

Yeah time is precious, but the good news is I'm working on this one :-)
You haven't by any chance coded this for Android yet, have you?

@danielgindi
Copy link
Collaborator

danielgindi commented Feb 20, 2017

There are a few coding style inconsistencies, but I'm not going to bug you about them right now; It's in the best interest of the users that we don't introduce any extra delays for this right now.

I'm actually surprised on how clean and little code this PR is, which means you understand how the library works.

@danielgindi
Copy link
Collaborator

There are a few bugs to polish though

@jainrohit2292
Copy link

I have updated my pods - pod update Charts but I can't find any property or method related to icon such as drawIconsEnabled

@askielboe
Copy link

This pull request hasn't been released yet.

To use non-released features on the master branch you can install using:

pod 'Charts', :git => 'git@github.com:danielgindi/Charts.git', :branch => 'master'

@sashalondon
Copy link

Any advice I have tried pod update and also askeilboe's advice but still cannot see this? I am using swift.
pod 'Charts', :git => 'git@github.com:danielgindi/Charts.git', :branch => 'master'

sean-saathoff-ua added a commit to underarmour/Charts that referenced this pull request Jun 2, 2017
* added test image to demo project

* all datasets via superclass: added var drawIconsEnabled, func isDrawIconsEnabled() and var iconOffset

* utils: drawImage(context:image:point:expectedSize:offset:)

* all renderers are drawing icon image if it is stored in entitie's data property and dataset allows

* some demos updated to toggle icons

* unit tests

* fix: misplacement of icon. Now icon is drawn correctly centered.

* allow to make vertical-only offset for icons on Pie and Radar charts (in addition to distance from center)

* drawBottomYLabelEntryEnabled (Closes ChartsOrg#1006)

* resetZoom() (Closes ChartsOrg#1179)

* Correctly position 0 in stacked bar (Closes ChartsOrg#1195, #closes ChartsOrg#1191)

* Update "Usage" section of README

Updates README.md to reflect changes made to "Build Options" in Xcode 8.2+.

* Removed embedded Realm leftover from repo

* Use realm-cocoa 2.1.1 for ObjectiveCSupport (Closes ChartsOrg#898, resolves ChartsOrg#1594)

* Updated project settings

* Add support for extensions

* Fix NSCopying implementation in CandleChartDataEntry

* Update RealmSwift Dependency

* update configurations for xcode 8.2

* fix a typo, as orientation is horizontal by default

* close ChartsOrg#2066

* update rakefile for ios 10.x simulators

* Adds NSPhotoLibraryUsageDescription to plist of ChartsDemo to prevent crash on iOS 10 on try to store shot of char to photo library.

* 'backgroundColor' is inaccessible due to 'internal' protection level on mac OS X ChartsOrg#2135

* Fixes index out of range crash.

* add github token to .travis.yml to fix builds?

* Code styling

* Fixed dependency of drawing values on isDrawIconsEnabled

* CGPoint is more appropriate here

* Do not repeat the scaling procedure if we can cache it

* Docs

* Deprecated weird constructor

* Use a designated icon property and constructors

* More refactoring to icons drawing

* Disabled icons by default in examples

* Fixes for icon tests

* Reverted basic test images from pr (ChartsOrg#1793)

* Improved feb29 formula

* Fixed the inconsistency of AxisMax and AxisMin

When enabled autoScale.
`data.calcMinMaxY(fromX: self.lowestVisibleX, toX: self.highestVisibleX)` is called after 
` _leftYAxisRenderer?.computeAxis(min: _leftAxis._axisMinimum, max: _leftAxis._axisMaximum, inverted: _leftAxis.isInverted)`
`_rightYAxisRenderer?.computeAxis(min: _rightAxis._axisMinimum, max: _rightAxis._axisMaximum, inverted: _rightAxis.isInverted)`
`_xAxisRenderer?.computeAxis(min: _xAxis._axisMinimum, max: _xAxis._axisMaximum, inverted: false)`

that means _leftAxisRenderer and _rightAxisRenderer will render AxisLines and Values based on old values of _axisMinimum, _axisMaximum

* Tell Travis to wait more time when building carthage

* Always respect isEnabled

* Consider _yAxis.isDrawLimitLinesBehindDataEnabled for radar chart

* Updated to use Realm version 2.4.3

* fix Xcode 8.3 compiler warnings

* loosen realm version requirement

* Updated build-dependencies.sh

* Remove line width minimum constraint

* Moved Realm stuff to https://github.com/danielgindi/ChartsRealm

* Added @discardableResult to silence warnings when it’s safe to ignore result

* gitignore updates

* Removed unrequited script

* v3.0.2

* Removed leftover scheme

* Removed leftover script from the combined Realm era

* Converted swift 3.0 DBL_MIN leftover
PeterSrost pushed a commit to sokol8/Charts that referenced this pull request Oct 31, 2018
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