willdale / SwiftUICharts

A charts / plotting library for SwiftUI. Works on macOS, iOS, watchOS, and tvOS and has accessibility features built in.
MIT License
860 stars 108 forks source link

Fix StackedBarChart Y Labels #55

Closed ataias closed 3 years ago

ataias commented 3 years ago

I tried my hand at the codebase regarding the issue I raised at https://github.com/willdale/SwiftUICharts/issues/54 . The tests are passing and I tested locally with the same demo, resulting in the labels showing the total sum:

image

I am not sure if this solution is adequate given the current architecture, or if it introduces any bugs. I am not acquainted with the codebase very much. I tried to limit the diff as much as possible.

willdale commented 3 years ago

Hey Ataias,

Thank you for pointing this out in #54 and thank you for the PR.

In your branch the the y axis labels now show the correct numbering, but I don't think the total height of bars are always correct.

If BarChartStyle -> topline is set to .maximumValue, everything is great. However if it's set to .maximum(of: 1400), the bars don't scale.

stackedDemo


A Possible Solution

When you raised the issue I had a look and came up with a different solution. It does involve some breaking changes but I feel that may be minor enough to push out without a whole version number change.

The changes are that MultiBarDataSets and MultiBarDataSet get renamed to StackedBarDataSets and StackedBarDataSet for the stacked bar chart and GroupedBarDataSets and GroupedBarDataSet for the grouped bar chart.

We can then do:

Sources/SwiftUICharts/Shared/Models/Protocols/SharedProtocolsExtensions.swift

extension CTMultiDataSetProtocol where Self == StackedBarDataSets {
    public func maxValue() -> Double {
        var setHolder : [Double] = []
        for set in self.dataSets {
            setHolder.append(set.dataPoints.reduce(0) { $0 + $1.value})
        }
        return setHolder.max { $0 < $1 } ?? 0
    }
}

extension CTMultiBarChartDataSet where Self == StackedBarDataSet {
    public func maxValue() -> Double  {
        return self.dataPoints.reduce(0) { $0 + $1.value}
    }
}

This means the compiler will find the relevant version of maxValue() when is is used throughout the code base, such as in View Modifiers.

All the preposed changes are in:

I'd welcome your feedback on it.

Thanks,

Will

ataias commented 3 years ago

@willdale Your solution is quite nice! I just noticed there are some typos in the docs related to bar charts. I took your commit and updated those docs in this PR. Could you check?

Also, I suggest a refactor and update of the docs for the two extensions you mentioned. The refactor is to make the code more clear. A variable like setHolder is quite generic, then I renamed to maxSums and removed the for loop for a more specific map and reduce. You can look at those changes in the last commit of this PR: https://github.com/willdale/SwiftUICharts/pull/55/commits/e8ba0539e389ce4111cbfdd1b406ef3f265bfa62

willdale commented 3 years ago

This is looking great, I haven't had a chance to pull and test yet but all looks good.

While we are changing the names of GroupedBarDataSets and StackedBarDataSets do you think we should split MultiBarChartDataPoint into:

While we don't need the change now, it adds more flexibility for the future.

Thoughts?

ataias commented 3 years ago

I like the suggested change @willdale . It is consistent with some other specializations so far and I think it will be more clear, besides flexible. I will have a go at it and update this in a moment.

ataias commented 3 years ago

@willdale I did the splitting and added a commit to this PR.

willdale commented 3 years ago

I've done some extra tidying.

If all looks good to you, I'm happy to merge this into main branch.

ataias commented 3 years ago

@willdale I looked at your edits and I think they all look good. I think it is fine to merge.