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

Bar chart custom value offset #4431

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

bivant
Copy link
Contributor

@bivant bivant commented Jul 30, 2020

Issue Link 🔗

Vertical version the HorizontalBarChartView issue #124
Possibly (in future) #777

Goals ⚽

Scope: BarChart (no horizontal bar logic for now)
Add ability to specify custom value offset for a value label (prior value is 4.5, what is still true by default)
When was implementing this feature I faced with the issue with low bars - text was unreadable when it has the same color as the background. So secondary value colors were added. After that I realized that inverting the value text color would be more universal and require almost no additional logic (except stacked, but that was later), so added this feature also.
Although coloring logic can be a separate PR, I have no interest in it without custom/flexible value positioning, so please not send me to split it right away.
While I need a non-stacked version I saw that drawing logic is very similar to "single bar" so added support of the features to it as well.

Implementation Details 🚧

BarChartDataProvider (affecting BarChartView):

    /// if set to true values those
    /// 1.do not fit into the value bar are drawn above them (below for negative), instead of partially inside and/or below (above if negative)
    /// 2.do not fit into the visible area above/below their bars are drawn inside
    var isDrawValueSideFlexible: Bool { get }
    /// distance from top (bottom in negative) for values drawn outside/inside the bar
    var valuesOffset: CGFloat { get }

BarChartView valuesOffset for a value label has same default value as before (4.5).
Setting a isDrawValueSideFlexible value resets the offset of vertical bars to zero as the padding is not needed because of the flexible position

ChartBaseDataSet:

    /// valueColors in case of soft positions in effect (BarCharView:isDrawValueSideFlexible)
    open var valueColorsSecondary = [NSUIColor]()

    /// Check if colors for the values for the DataSet are distinct from the background/bar color and apply inverted color if opposite
    open var valueColorsAdjustment: Bool = false

valueColorsSecondary are applied for the values affected by isDrawValueSideFlexible only.
valueColorsAdjustment in effect for all value labels in the chart if enabled

Testing Details 🔍

Tried to add tests for all the edge cases I could think of: custom value offset, flexible positioning, colors inverting+secondary, combination of them. Stacked and non-stacked versions.

Additionally add an option to fallback to the outside value placement in case of the internal placement logic of the bar values + too low height of the bar to fit in the value
…ndle position fallback for the top edge

Rename fallback value placement parameter name to the "isDrawValueSideFlexible".
Handle fallback value placement for the bars those reach top edge (was only for the bottom/zero line).
Add new options to the demo bar controllers (BarChar, PositiveNegativeBarChar)
…ue colors

Move the value display code to a function.  Stacked bar chart: correct text color (was using a dataSet index instead of a bar index), enable new features in the demo controller
@bivant
Copy link
Contributor Author

bivant commented Jul 31, 2020

I am ready to port the changes to the HorizontalBar if they are accepted.
These issues may be fixed:
#709
#124

For the #777 need guidance where to put the parameters as there should be a lot of them - position factor, offset, rotation, alignment, maybe something else.

# Conflicts:
#	Source/Charts/Renderers/BarChartRenderer.swift
@bivant
Copy link
Contributor Author

bivant commented Dec 14, 2020

Seems like the reference images for the new tests need to be updated, that was done for existing tests in master already after the merge with 4.0 branch.

# Conflicts:
#	Source/Charts/Renderers/BarChartRenderer.swift
# Conflicts:
#	Tests/ChartsTests/BarChartTests.swift
#	Tests/ChartsTests/__Snapshots__x86__/BarChartTests/testDefaultNotDrawValueAboveBarsFlexibleInvertColor_iOS_375_0_667_0@2x.png
#	Tests/ChartsTests/__Snapshots__x86__/BarChartTests/testDefaultNotDrawValueAboveBarsFlexibleInvertColor_tvOS_1920_0_1080_0@1x.png
#	Tests/ChartsTests/__Snapshots__x86__/BarChartTests/testDefaultNotDrawValueAboveBarsFlexibleSecondaryColor_iOS_375_0_667_0@2x.png
#	Tests/ChartsTests/__Snapshots__x86__/BarChartTests/testDefaultNotDrawValueAboveBarsFlexibleSecondaryColor_tvOS_1920_0_1080_0@1x.png
#	Tests/ChartsTests/__Snapshots__x86__/BarChartTests/[email protected]
#	Tests/ChartsTests/__Snapshots__x86__/BarChartTests/testDefaultNotDrawValueAboveBarsFlexible_tvOS_1920_0_1080_0@1x.png
#	Tests/ChartsTests/__Snapshots__x86__/BarChartTests/[email protected]
#	Tests/ChartsTests/__Snapshots__x86__/BarChartTests/testDefaultValuesFlexibleSecondaryColor_tvOS_1920_0_1080_0@1x.png
#	Tests/ChartsTests/__Snapshots__x86__/BarChartTests/[email protected]
#	Tests/ChartsTests/__Snapshots__x86__/BarChartTests/[email protected]
#	Tests/ChartsTests/__Snapshots__x86__/BarChartTests/[email protected]
#	Tests/ChartsTests/__Snapshots__x86__/BarChartTests/testNegativeValuesFlexibleSecondaryColor_tvOS_1920_0_1080_0@1x.png
#	Tests/ChartsTests/__Snapshots__x86__/BarChartTests/testNotDrawValueAboveBarsNegativeValuesFlexibleSecondaryColor_iOS_375_0_667_0@2x.png
#	Tests/ChartsTests/__Snapshots__x86__/BarChartTests/testNotDrawValueAboveBarsNegativeValuesFlexibleSecondaryColor_tvOS_1920_0_1080_0@1x.png
#	Tests/ChartsTests/__Snapshots__x86__/BarChartTests/[email protected]
#	Tests/ChartsTests/__Snapshots__x86__/BarChartTests/[email protected]
#	Tests/ChartsTests/__Snapshots__x86__/BarChartTests/[email protected]
#	Tests/ChartsTests/__Snapshots__x86__/BarChartTests/testStackedAndRegularNegativeDrawValues_tvOS_1920_0_1080_0@1x.png
#	Tests/ChartsTests/__Snapshots__x86__/BarChartTests/[email protected]
#	Tests/ChartsTests/__Snapshots__x86__/BarChartTests/testStackedAndRegularPositiveDrawValues_tvOS_1920_0_1080_0@1x.png
#	Tests/ChartsTests/__Snapshots__x86__/BarChartTests/[email protected]
#	Tests/ChartsTests/__Snapshots__x86__/BarChartTests/testStackedAndRegularsDrawValuesFlexible_tvOS_1920_0_1080_0@1x.png
#	Tests/ChartsTests/__Snapshots__x86__/BarChartTests/testStackedAndRegularsNotDrawValueAboveBarsFlexible_iOS_375_0_667_0@2x.png
#	Tests/ChartsTests/__Snapshots__x86__/BarChartTests/testStackedAndRegularsNotDrawValueAboveBarsFlexible_tvOS_1920_0_1080_0@1x.png
#	Tests/ChartsTests/__Snapshots__x86__/BarChartTests/testStackedAndStackSingleNegativeDrawValues_iOS_375_0_667_0@2x.png
#	Tests/ChartsTests/__Snapshots__x86__/BarChartTests/testStackedAndStackSingleNegativeDrawValues_tvOS_1920_0_1080_0@1x.png
#	Tests/ChartsTests/__Snapshots__x86__/BarChartTests/testStackedAndStackSinglePositiveDrawValues_iOS_375_0_667_0@2x.png
#	Tests/ChartsTests/__Snapshots__x86__/BarChartTests/testStackedAndStackSinglePositiveDrawValues_tvOS_1920_0_1080_0@1x.png
#	Tests/ChartsTests/__Snapshots__x86__/BarChartTests/testStackedAndStackSinglesDrawValueInvertColors_iOS_375_0_667_0@2x.png
#	Tests/ChartsTests/__Snapshots__x86__/BarChartTests/testStackedAndStackSinglesDrawValueInvertColors_tvOS_1920_0_1080_0@1x.png
#	Tests/ChartsTests/__Snapshots__x86__/BarChartTests/testStackedAndStackSinglesDrawValuesFlexible_iOS_375_0_667_0@2x.png
#	Tests/ChartsTests/__Snapshots__x86__/BarChartTests/testStackedAndStackSinglesDrawValuesFlexible_tvOS_1920_0_1080_0@1x.png
#	Tests/ChartsTests/__Snapshots__x86__/BarChartTests/testStackedAndStackSinglesNotDrawValueAboveBarsFlexibleInvertColors_iOS_375_0_667_0@2x.png
#	Tests/ChartsTests/__Snapshots__x86__/BarChartTests/testStackedAndStackSinglesNotDrawValueAboveBarsFlexibleInvertColors_tvOS_1920_0_1080_0@1x.png
#	Tests/ChartsTests/__Snapshots__x86__/BarChartTests/testStackedAndStackSinglesNotDrawValueAboveBarsFlexible_iOS_375_0_667_0@2x.png
#	Tests/ChartsTests/__Snapshots__x86__/BarChartTests/testStackedAndStackSinglesNotDrawValueAboveBarsFlexible_tvOS_1920_0_1080_0@1x.png
#	Tests/ChartsTests/__Snapshots__x86__/BarChartTests/[email protected]
#	Tests/ChartsTests/__Snapshots__x86__/BarChartTests/testStackedDrawValuesFlexibleInvertColors_tvOS_1920_0_1080_0@1x.png
#	Tests/ChartsTests/__Snapshots__x86__/BarChartTests/[email protected]
#	Tests/ChartsTests/__Snapshots__x86__/BarChartTests/[email protected]
# Conflicts:
#	Source/Charts/Renderers/BarChartRenderer.swift
… into BarChart_CustomValueOffset

# Conflicts:
#	Source/Charts/Renderers/BarChartRenderer.swift
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.

1 participant