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

Allow rotating Y axis labels and fix bug when that is the case #7

Merged

Conversation

interstateone
Copy link
Contributor

This change came from a use case where we had Y axis title labels on the left and right that corresponded with two different data sets shown at the same time. We wanted both labels' baselines to be on the inside (facing the chart) and so we needed the right-side title label to be rotated 90 degrees instead of the standard -90.

This change creates a label settings subclass for the Y axis that has the same default values as before (particularly the -90 degree rotation) but allows changing the rotation value. When the value is rotated, the Y layer now uses the correct dimension of the axis title label in order to calculate the width. The label also correctly calculates its rotated bounds (previously it treated degrees as radians).

Create label settings subclass for Y axis that has same default values
as before but allows changing the rotation value. When the value is
rotated, the Y layer now uses the correct dimension of the axis title
label in order to calculate the width.
@ivnsch
Copy link
Owner

ivnsch commented Jul 1, 2015

Very cool, thanks!

The only thing I would implement differently is the ChartAxisYLabelSettings class to provide the -90 default. I would create a method to return ChartLabelSettings with overwritten rotation settings. And put the -90 default in an extension, using this method. This way instead of having to pass all the parameters again to create the y-axis label you can just write labelSettings.defaultVertical(). Something like:

public class ChartLabelSettings {
    let font: UIFont
    let fontColor: UIColor
    let rotation: CGFloat
    let rotationKeep: ChartLabelDrawerRotationKeep
    let shiftXOnRotation: Bool

    public init(font: UIFont = UIFont.systemFontOfSize(14), fontColor: UIColor = UIColor.blackColor(), rotation: CGFloat = 0, rotationKeep: ChartLabelDrawerRotationKeep = .Center, shiftXOnRotation: Bool = true) {
        self.font = font
        self.fontColor = fontColor
        self.rotation = rotation
        self.rotationKeep = rotationKeep
        self.shiftXOnRotation = shiftXOnRotation
    }

    public func copy(font: UIFont? = nil, fontColor: UIColor? = nil, rotation: CGFloat? = nil, rotationKeep: ChartLabelDrawerRotationKeep? = nil, shiftXOnRotation: Bool? = nil) -> ChartLabelSettings {
        return ChartLabelSettings(
            font: font ?? self.font,
            fontColor: fontColor ?? self.fontColor,
            rotation: rotation ?? self.rotation,
            rotationKeep: rotationKeep ?? self.rotationKeep,
            shiftXOnRotation: shiftXOnRotation ?? self.shiftXOnRotation)
    }
}

public extension ChartLabelSettings {
    public func defaultVertical() -> ChartLabelSettings {
        return self.copy(rotation: -90)
    }
}

Also, the examples have to be adjusted. But I can do this after the merge.

ivnsch added a commit that referenced this pull request Jul 3, 2015
…ionOverride

Allow rotating Y axis labels and fix bug when that is the case
@ivnsch ivnsch merged commit ec5d5e8 into ivnsch:master Jul 3, 2015
@interstateone
Copy link
Contributor Author

This way instead of having to pass all the parameters again to create the y-axis label you can just write labelSettings.defaultVertical()

Our use case had us creating different label settings per axis anyways, so the more specific subclass was more natural. I see what you mean in the examples though.

👍 Thanks for the merge!

@interstateone interstateone deleted the AllowYAxisTitleLabelRotationOverride branch July 3, 2015 16:01
@ivnsch
Copy link
Owner

ivnsch commented Jul 3, 2015

Thanks to you for the contribution!

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.

2 participants