willdale / SwiftUICharts

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

Add .position DisplayValue to show POI Labels at a certain position inside the chart + Custom Shapes for POI Labels #161

Closed kunalverma25 closed 2 years ago

kunalverma25 commented 2 years ago

Addition of .oppositeYAxis DisplayValue Enum for showing POI labels at the opposite end of yAxis Enhancement for #160 and #162

Enhancements/Fixes -

kunalverma25 commented 2 years ago

Hey @willdale This PR adds an additional option for the POI label position for the opposite end of the axis, w/o modifying current implementation. Plus adds input for custom shape, icon view and padding for POS labels for more configurability.

kunalverma25 commented 2 years ago

@willdale Can these enhancements be added in 2.x version? They don’t break/modify current implementation.

willdale commented 2 years ago

@kunalverma25 I'm happy in principal with the changes for v2, they will likely not get ported forward into v3 as the implementation has completely changed for the POI.

I ran out of time to do a full review before work tis morning but one thing that jumped out was the use of AnyView. It is generally not recommended to use AnyView - See Natascha Fadeeva's article or swiftbysundell's.

kunalverma25 commented 2 years ago

@willdale I’ll be more than happy to add these changes to the new implementation of the POIs when finalized. As for AnyView I get that it prevents optimizations and causes issues in updates as every view is of the same type but the use case here itself doesn’t require those as the view itself(icon) is just statically passed and the redrawing depends on graph data. Besides adding the same functionality via ViewBuilders will only create unnecessary bridging code from ViewModifiers to Protocol Conformances and force to manually typecast the passed in views which basically defeats the whole purpose of not using AnyView.

willdale commented 2 years ago

Look back at the issue #160 you raised, is the labelIcon for adding the '$' symbol? If so then you could use the number formatter.

kunalverma25 commented 2 years ago

I also wanted to add some image, sf-symbol and/or anything non-text.

kunalverma25 commented 2 years ago

@willdale Updated the PR.. Removed the labelIcon for now + position is now configurable.