Axis rendering improvements #3
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
7ccc39c to
314711b
Compare
This still has test failures and an introduced issue with y-axis rendering that will be fixed in the next commit
This improves the Y axis rendering, fixing issues introduced in the previous commit as well as providing better range calculation logic. Tests are still failing as part of this commit, although these changes are believed to be good more validation is needed. The next commit should cleanup any remaining issues and fix the unit tests.
In addition axis asserts were updated as the updated SVG's match the expected rendering changes. Other tests show areas for potential improvement
This improves the y axis by picking a max value that results in more round intervals. In addition the label counts were better defined in code, and the line chart tests were updated
This change includes performance improvements by reducing the looping for min padding calculation, and only calculating the padded max value once. There remains an issue with bar chart rendering that needs to be addressed.
… mark points are used
These tests looked good only by chance due to the prior default of 6 for the axis labels. We need to set the divide count based on the categories rather than just use the default.
This adds the `LabelSkipCount` configuration as a way to reduce the amount of y axis labels, while maintaining the same `LabelCount` number of horizontal lines. As part of testing around this change it was found that the Unit parameter was not handled in the best way possible. A long comment in charts.go decribes the circular relationship between the axis range and label count (particularly if a unit is configured).
314711b to
0b35ded
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR encapsulates a significant rework to how the X and Y axis are rendered. Commits represents the incremental changes, but these changes are really only correct and functional within the last commit, a variety of bugs were introduced and later fixed as part of this work.
API Changes
The configuration for label frequency has changed. To configure the labels of your chart use the following values:
Unit- Unit specifies the increment at which each label should be shown. This value is only a recommendation as the start and end of the range will always be displayed.LabelCount- This directly specifies how many labels should be rendered, and will take precedent ifUnitis also configured. Labels will be evenly spaced across the axis range.LabelSkipCount- A new Y-Axis option to allow horizontal lines without a y-axis label to produce simpler graphs.RangeValuePaddingScale- If set you can influence how much value padding should be added to the min and max of the y-axis.MaxorMinvalue is set on the y-axis, there will not be any automatic padding adjustments (unless necessary to meet the configured Unit).SplitNumber- REMOVED, use the values described above.Improvements
These changes are a much more complete version of the update original proposed to the parent project here: vicanso/go-charts#56
Those changes were reverted from the upstream project. As discussed in the original PR, I believe that a semi-workable solution would be possible, however after looking deeper I believe that API changes were necessary to fully address this design. The use of
Unitas a primary label interval means that you could provide configurations which don't divide evenly into the range. That is why in our API changes theunitis only a recommendation.There were other technical challenges in the original implementation, but the API based ones were the most significant reason for starting this fork.
This PR represents the first significant diversion from the parent vicanso/go-charts project. I have huge appreciation for the work @vicanso has started, and hope I can provide my own spin on his start to improve the API and styling of wcharczuk/go-chart.
I plan to make additional changes to this fork following this PR. I have significant experience using the parent fork's implementation and I want to incorporate improvements based off my experiences. Specifically API changes, some minor chart styling changes, and a few small feature additions. Those changes will be coming in future pull requests!