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'

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.