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

Named axis support #2387

Closed
wants to merge 10 commits into from
Closed

Named axis support #2387

wants to merge 10 commits into from

Conversation

thierryH91200
Copy link
Contributor

capture d ecran 2017-04-24 a 20 40 57

capture d ecran 2017-04-24 a 20 45 41

capture d ecran 2017-04-24 a 20 52 39

add

     axis.nameAxisFont = NSUIFont.systemFont(ofSize: CGFloat(14.0))
       axis.nameAxis = "Day"
       axis.nameAxisTextColor = #colorLiteral(red: 1, green: 0.1491314173, blue: 0, alpha: 1)
       axis.nameAxisEnabled = true

I think it should have success like PR

@liuxuan30
Copy link
Member

liuxuan30 commented Apr 25, 2017

Wow, you have been helping a lot!
some questions,

  1. do you consider adding some offset for the left/right of the name? (bottom/top for x axis).
  2. seems you forgot horizontal bar chart and radar/pie? Seeing separate axis renderer for them. Maybe we can consolidate in a common place so we can reuse them

Supporting only 90 degree at beginning is fine and we can handle the new test cases later

}
else
{
if labelPosition == .outsideChart
{
textAlign = .left
xPos = viewPortHandler.contentRight + xoffset

xPosName = viewPortHandler.chartWidth - 5
Copy link
Member

Choose a reason for hiding this comment

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

what is 5 here?

@@ -44,6 +44,7 @@ open class YAxisRenderer: AxisRendererBase
let labelPosition = yAxis.labelPosition

var xPos = CGFloat(0.0)
var xPosName = CGFloat(0.0)
Copy link
Member

Choose a reason for hiding this comment

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

xPosName is a little confusing here. how about xNamePos or other style?

if xAxis.isEnabled && xAxis.isDrawLabelsEnabled
{
let xlabelheight = xAxis.labelRotatedHeight + xAxis.yOffset

var size = NSSize()
Copy link
Member

Choose a reason for hiding this comment

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

Travis CI for tvOS and iOS fails here, maybe NSUISize()?

@codecov-io
Copy link

codecov-io commented Apr 25, 2017

Codecov Report

Merging #2387 into master will increase coverage by 0.01%.
The diff coverage is 18.73%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2387      +/-   ##
==========================================
+ Coverage   19.66%   19.67%   +0.01%     
==========================================
  Files         112      112              
  Lines       13713    13910     +197     
==========================================
+ Hits         2696     2737      +41     
- Misses      11017    11173     +156
Impacted Files Coverage Δ
Source/Charts/Components/AxisBase.swift 48.73% <ø> (ø) ⬆️
...ts/Renderers/XAxisRendererHorizontalBarChart.swift 0% <0%> (ø) ⬆️
Source/Charts/Charts/HorizontalBarChartView.swift 0% <0%> (ø) ⬆️
...ts/Renderers/YAxisRendererHorizontalBarChart.swift 0% <0%> (ø) ⬆️
Source/Charts/Renderers/YAxisRenderer.swift 53.16% <21.21%> (-2.79%) ⬇️
Source/Charts/Renderers/XAxisRenderer.swift 58.6% <37.5%> (-0.48%) ⬇️
Source/Charts/Charts/BarLineChartViewBase.swift 23.7% <38.57%> (+0.26%) ⬆️
Source/Charts/Charts/ChartViewBase.swift 24.23% <0%> (-0.06%) ⬇️

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 dc1e965...7f515a2. Read the comment docs.

@thierryH91200
Copy link
Contributor Author

thierryH91200 commented Apr 25, 2017

1 do you consider adding some offset for the left/right of the name? (bottom/top for x axis).
yes i add offset depending on the height of the font

capture d ecran 2017-04-25 a 08 00 24

2 seems you forgot horizontal bar chart and radar/pie? Seeing separate axis renderer for them. Maybe we can consolidate in a common place so we can reuse them
I'll take care of it

PieChartView
/// This will throw an exception, PieChart has no XAxis object.
open override var xAxis: XAxis
{
fatalError("PieChart has no XAxis")
}

@liuxuan30
Copy link
Member

Oh I forgot. Seems we can ignore pie/radar chart's for now, since their axis is not normal, and we don't have requests for displaying name?

@liuxuan30
Copy link
Member

@thierryH91200 I was thinking, if there are demands, such as the inter space between the name and axis line? I see you are just using existing offsetLeft/right/top/bottom for the space between the chart view boundary and the axis name.

I am not sure if we should use separate "axisName Left/Right/Top/Bottom/ Space" for them. Any other people who have ideas?

@thierryH91200
Copy link
Contributor Author

add HorizontalBarChart

capture d ecran 2017-04-26 a 15 09 50

@thierryH91200
Copy link
Contributor Author

I was not very pleased with me
Complete rewrite of name Axis
Creation of 4 rectangles to preserve the place of the name of the axis
This avoids overlays and has a more readable code

     Open var nameRectBottom = CGRect ()
     Open var nameRectTop = CGRect ()
     Open var nameRectLeft = CGRect ()
     Open var nameRectRight = CGRect ()

For BarLineChartViewBase
NameRectBottom and nameRectTop are associated with xAxis
NameRectLeft is associated with leftAxis
NameRectRight are associated with rightAxis

For HorizontalBarChartView
NameRectLeft and nameRectRight are associated with xAxis
NameRectBottom is associated with rightAxis
NameRectTop is associated with leftAxis

@liuxuan30
Copy link
Member

LOL, take your time:)

@hungcaobss
Copy link

Hi, was this supported for iOS? I have checked in the latest version but don't see it

@thierryH91200
Copy link
Contributor Author

not reviewed yet.

capture d ecran 2017-05-22 a 08 06 04

@seenoevo
Copy link

please add this feature @danielgindi :)

@dhaneshgosai
Copy link

Hello @danielgindi,

Is this feature reviewed ?

@shashidhar34
Copy link

is this feature in the current released version?

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.

7 participants