Skip to content

Refactor charts, BarChart and LineChart layout fixes#7345

Merged
henon merged 9 commits intoMudBlazor:devfrom
chausner:charts-refactoring
Aug 18, 2023
Merged

Refactor charts, BarChart and LineChart layout fixes#7345
henon merged 9 commits intoMudBlazor:devfrom
chausner:charts-refactoring

Conversation

@chausner
Copy link
Contributor

@chausner chausner commented Aug 13, 2023 •
edited
Loading

Description

This is a refactoring of the chart code with some minor fixes and improvements, continued from #2209. Summary of changes:

  • Moved constants to the top
  • Split up the very long OnParametersSet method into smaller separate methods
  • Marked ILineInterpolator.InterpolationRequired and NoInterpolation as obsolete since no longer used
  • Minor fixes, e.g. replaced ((int)(maxY / gridYUnits)) + 1; by (int)Math.Ceiling(maxY / gridYUnits);. The former rounds up even if maxY is a multiple of gridYUnits, which is probably undesired here.
  • The variables numHorizontalLines and numVerticalLines now really do denote the respective number of lines, before it was number of lines - 1
  • The "safeguard against millions of gridlines which might arise with very high values" which only existed in LineChart has been copied to BarChart
  • Performance optimization: avoid population of XValues and YValues arrays when interpolation is disabled
  • Performance optimization: use StringBuilder to construct the SVG data

How Has This Been Tested?

  • Tested the changes visually using the Unit test viewer charts page
  • The existing unit tests have been adapted

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Before:
old
After (note the y axis range of the bar chart and the x axis range of the line chart):
new2

Checklist:

  • The PR is submitted to the correct branch (dev).
  • My code follows the code style of this project.
  • I've added relevant tests.

@github-actions github-actions bot added bug Unexpected behavior or functionality not working as intended PR: needs review labels Aug 13, 2023
@chausner chausner mentioned this pull request Aug 13, 2023
Closed
@chausner chausner changed the title Refactor charts, tweak chart layout Refactor charts, BarChart and LineCart layout fixes Aug 13, 2023
@chausner chausner changed the title Refactor charts, BarChart and LineCart layout fixes Refactor charts, BarChart and LineChart layout fixes Aug 13, 2023
@codecov
Copy link

codecov bot commented Aug 13, 2023 •
edited
Loading

Codecov Report

Patch coverage: 97.61% and project coverage change: -0.04% ⚠️

Comparison is base (4cf042d) 90.62% compared to head (5729ed5) 90.59%.
Report is 3 commits behind head on dev.

❗ Current head 5729ed5 differs from pull request most recent head a561c16. Consider uploading reports for the commit a561c16 to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##              dev    #7345      +/-   ##
==========================================
- Coverage   90.62%   90.59%   -0.04%     
==========================================
  Files         427      427              
  Lines       15174    15157      -17     
==========================================
- Hits        13752    13731      -21     
- Misses       1422     1426       +4
Files Changed Coverage Δ
src/MudBlazor/Components/Chart/Charts/Donut.razor 100.00% <ø> (ø)
...mponents/Chart/Interpolation/SplineInterpolator.cs 90.69% <ø> (ø)
src/MudBlazor/Components/Chart/Charts/Bar.razor.cs 96.73% <95.31%> (-3.27%) ⬇️
...rc/MudBlazor/Components/Chart/Charts/Line.razor.cs 98.36% <98.92%> (-0.89%) ⬇️
...c/MudBlazor/Components/Chart/Charts/Donut.razor.cs 100.00% <100.00%> (ø)
src/MudBlazor/Components/Chart/Charts/Pie.razor.cs 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ScarletKuro ScarletKuro requested a review from henon August 13, 2023 20:43
@ScarletKuro
Copy link
Member

ScarletKuro commented Aug 13, 2023

Hi.
I don't see the justification of removing InterpolationRequired and NoInterpolation, even if they are not used / not required anymore, removing any public API (property, classes etc) is a breaking change, which means your PR will be on hold until next major version, I would value more pull requests that are not breaking.

@chausner
Copy link
Contributor Author

chausner commented Aug 14, 2023

Hi. I don't see the justification of removing InterpolationRequired and NoInterpolation, even if they are not used / not required anymore, removing any public API (property, classes etc) is a breaking change, which means your PR will be on hold until next major version, I would value more pull requests that are not breaking.

I guess we could mark that property and class as deprecated and remove it with version 7. I'd prefer that over deferring this PR until version 7.

I wonder if maybe all the interpolation functionality should better not have been made part of the public API. It might be useful if somebody wants to write their own chart control and reuse the interpolation functionality but otherwise, this logic could be seen as an implementation detail of the inbuilt charts, no?

@ScarletKuro
Copy link
Member

ScarletKuro commented Aug 14, 2023

I guess we could mark that property and class as deprecated and remove it with version 7. I'd prefer that over deferring this PR until version 7.

Yeah, that would be the best way to address it. What do you think @henon?

@henon
Copy link
Contributor

henon commented Aug 17, 2023 •
edited
Loading

Sorry for the late reply, but yes, obsoleting and removing in v7 is the way to go.

PS: if you like you can even PR the removal against our v7 branch in a separate PR

@ScarletKuro ScarletKuro added the needs: changes A maintainer has asked for further modifications to be made to this pull request label Aug 17, 2023
ScarletKuro
ScarletKuro requested changes Aug 17, 2023
View reviewed changes
@henon henon merged commit 85f4a9d into MudBlazor:dev Aug 18, 2023
@ScarletKuro ScarletKuro removed needs: changes A maintainer has asked for further modifications to be made to this pull request PR: needs review labels Aug 19, 2023
@chausner chausner deleted the charts-refactoring branch August 21, 2023 20:39
ilovepilav pushed a commit to ilovepilav/MudBlazor that referenced this pull request Nov 25, 2023
@chausner @ilovepilav
Refactor charts, BarChart and LineChart layout fixes (MudBlazor#7345)
014fa30 
* Refactor charts

* Fix ComputeUnitsAndNumberOfLines for empty series/data

* Fix broken Bar and Line charts

* Minor tweaks

* Add missing usings

* Fix BarChart and LineChart tests

* Add NoInterpolation and InterpolationRequired back and mark as obsolete

* Fix compilation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ScarletKuro ScarletKuro ScarletKuro approved these changes

@henon henon Awaiting requested review from henon

Assignees

No one assigned

Labels

bug Unexpected behavior or functionality not working as intended

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@chausner @ScarletKuro @henon