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

Fix CombinedChartView not draw markers #2702

Merged
merged 3 commits into from
Sep 5, 2017
Merged

Conversation

xzysun
Copy link

@xzysun xzysun commented Aug 11, 2017

Try to fix issue #2692.

return nil
}

let data = dataObjects[highlight.dataIndex]
Copy link
Member

Choose a reason for hiding this comment

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

you can leverage dataByIndex() to remove the need to use dataObjects

/// - Returns: dataset related to highlight
open func getDataSetByHighlight(_ highlight: Highlight) -> IChartDataSet!
{
let dataObjects = allData
Copy link
Member

Choose a reason for hiding this comment

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

seems unnecessary to declare dataObjects

@codecov-io
Copy link

codecov-io commented Aug 14, 2017

Codecov Report

Merging #2702 into master will increase coverage by 0.02%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2702      +/-   ##
==========================================
+ Coverage   19.65%   19.67%   +0.02%     
==========================================
  Files         112      113       +1     
  Lines       13717    13772      +55     
==========================================
+ Hits         2696     2710      +14     
- Misses      11021    11062      +41
Impacted Files Coverage Δ
Source/Charts/Charts/CombinedChartView.swift 0% <0%> (ø) ⬆️
...a/Implementations/Standard/CombinedChartData.swift 0% <0%> (ø) ⬆️
...ource/Charts/Renderers/ChartDataRendererBase.swift 47.05% <0%> (-2.95%) ⬇️
Tests/Charts/ChartUtilsTests.swift 100% <0%> (ø) ⬆️
.../Charts/Renderers/HorizontalBarChartRenderer.swift 0% <0%> (ø) ⬆️
Tests/Charts/Snapshot.swift 100% <0%> (ø)

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 e72f280...59218f0. Read the comment docs.

@liuxuan30
Copy link
Member

liuxuan30 commented Aug 14, 2017

I have restarted the failed task on tvOS, looks like passed.
Also, have you tested it thoroughly?

@xzysun
Copy link
Author

xzysun commented Aug 15, 2017

I‘ve test the function on my forked branch, it works fine when drawing markers on a combined chart view.

For the code style question, I'm not familiar with swift, so the added function is imitated from existing ones...

@liuxuan30
Copy link
Member

OK, I need you review my comments and if it makes sense please update your code and I can merge then.

@liuxuan30 liuxuan30 self-assigned this Aug 29, 2017
@liuxuan30
Copy link
Member

If you don't have time that's fine. I will find a slot this week to update, test and merge

@liuxuan30
Copy link
Member

liuxuan30 commented Sep 5, 2017

@xzysun Thanks for a great work!

I tested within ChartsDemo combined chart with balloon marker, works fine

    BalloonMarker *marker = [[BalloonMarker alloc]
                             initWithColor: [UIColor colorWithWhite:180/255. alpha:1.0]
                             font: [UIFont systemFontOfSize:12.0]
                             textColor: UIColor.whiteColor
                             insets: UIEdgeInsetsMake(8.0, 8.0, 20.0, 8.0)];
    marker.chartView = _chartView;
    marker.minimumSize = CGSizeMake(80.f, 40.f);
    _chartView.marker = marker;

to test put above code in CombinedChartViewController -> viewDidLoad()

@liuxuan30
Copy link
Member

liuxuan30 commented Sep 5, 2017

If someone asks about why the first and last marker position has offset, it's because offsetForDrawing() is force setting

else if chart != nil && point.x + width + offset.x > chart!.bounds.size.width
{
    offset.x = chart!.bounds.size.width - point.x - width
}

@liuxuan30 liuxuan30 merged commit f9eea57 into ChartsOrg:master Sep 5, 2017
@StefaniOSApps
Copy link

StefaniOSApps commented Apr 18, 2018

look at #3404

PeterSrost pushed a commit to sokol8/Charts that referenced this pull request Oct 31, 2018
Fix CombinedChartView not draw markers
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.

4 participants