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

BUGFIX Fix bargraph overflow #1468

Merged
merged 5 commits into from
Nov 13, 2023
Merged

Conversation

bobatsar
Copy link
Contributor

  • If bargraphs overflow (more bars than the available space) with BarChartAlignment.start, BarChartAlignment.center or BarChartAlignment.end, use BarChartAlignment.spaceEvenly to shrink all bars to the available space
  • Update documentation for BarChartData.groupsSpace

If the automatic approach to shrink all bars is not desired, another option would be to add other alignments e.g. BarChartAlignment.startShrink or another parameter to enable shrink on overflow.

Florian Zierer and others added 2 commits October 26, 2023 16:10
- If bargraphs overflow with BarChartAlignment.start,
  BarChartAlignment.center or BarChartAlignment.end, use
  BarChartAlignment.spaceEvenly to shrink all bars to the available space
- Update documentation for BarChartData.groupsSpace
Copy link

codecov bot commented Nov 5, 2023

Codecov Report

Merging #1468 (f13bbf8) into master (b3529d3) will decrease coverage by 0.22%.
The diff coverage is 81.25%.

❗ Current head f13bbf8 differs from pull request most recent head 6d5644b. Consider uploading reports for the commit 6d5644b to get more accurate results

@@            Coverage Diff             @@
##           master    #1468      +/-   ##
==========================================
- Coverage   86.67%   86.45%   -0.22%     
==========================================
  Files          45       45              
  Lines        3016     2998      -18     
==========================================
- Hits         2614     2592      -22     
- Misses        402      406       +4     
Files Coverage Δ
lib/src/extensions/bar_chart_data_extension.dart 94.91% <81.25%> (-5.09%) ⬇️

... and 4 files with indirect coverage changes

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

@imaNNeo
Copy link
Owner

imaNNeo commented Nov 5, 2023

Hi, please read the Contributing guideline carefully

  • You need to update the unit-tests (run make sure to make sure that everything is working well)
  • You need to update the Changelog.md

@bobatsar
Copy link
Contributor Author

@imaNNeo I just added a widget test (with golden files). I didn't find another way to check if the bars are drawn outside of the diagram. I can't check for the bars with a finder due to the fact that they are rendered on the canvas.
If you know a better approach, please tell me and I will fix it.

@imaNNeo
Copy link
Owner

imaNNeo commented Nov 13, 2023

Nice, I need to learn more about the golden tests.
Thanks for your contribution!

@imaNNeo imaNNeo merged commit 24d0fc5 into imaNNeo:master Nov 13, 2023
2 checks passed
@bobatsar
Copy link
Contributor Author

Hey @imaNNeo , thanks for merging.

For golden tests I can also recommend the following package.
I use it quite extensively e.g. for testing different device sizes.

https://pub.dev/packages/golden_toolkit

@k0psutin
Copy link
Contributor

k0psutin commented Nov 19, 2023

Golden tests are a good way to see if something has changed drastically. It's just that golden tests give different results on different OS - I think there should be some fault tolerance (0.2 - 0.3%) for the tests. At least for me, because I use Windows, I get failed tests on master because 0.15 - 0.2% differences.

════════════════════════════════════════════════════════════════════════════════════════════════════
00:02 +41: D:/Code/fl_chart/test/chart/bar_chart/bar_chart_alignment_widget_test.dart: Barchart alignment overflow test
══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞════════════════════════════════════════════════════
The following assertion was thrown while running async test code:
Golden "golden/BarChartAlignment.start_20.0.png": Pixel test failed, 0.20% diff detected.
Failure feedback can be found at /D:/Code/fl_chart/test/chart/bar_chart/failures

When the exception was thrown, this was the stack:
#0      LocalFileComparator.compare (package:flutter_test/src/_goldens_io.dart:101:7)
<asynchronous suspension>
<asynchronous suspension>
(elided one frame from package:stack_trace)

@k0psutin
Copy link
Contributor

See https://github.com/flutter/flutter/wiki/Writing-a-golden-file-test-for-package:flutter#creating-a-new-golden-file-test

Quote from link:

Golden tests may be executed locally on Linux, MacOS, and Windows platforms. All reference images can be found at Flutter Gold baselines. Some tests may have multiple golden masters for a given test, to account for rendering differences across platforms. The parameter set for each image is listed in each image digest to differentiate renderings.

@imaNNeo
Copy link
Owner

imaNNeo commented Nov 21, 2023

@k0psutin You're right, I get about 93% diff on my machine 😆
image

@bobatsar
Copy link
Contributor Author

@imaNNeo sorry if the golden test make issues.

@k0psutin
There is a way to add a fault tolerance.

https://stackoverflow.com/a/73048760/3607358 or
https://medium.com/mobilepeople/how-to-add-difference-tolerance-to-golden-tests-on-flutter-2d899c8baad2

I use this also for my tests in another project.
I could add this in the next days if it is wanted.

@imaNNeo but 93% diff is another level. There must be something completely off on your side. Did you check the failure images?

@k0psutin
Copy link
Contributor

k0psutin commented Nov 21, 2023

@k0psutin
There is a way to add a fault tolerance.

https://stackoverflow.com/a/73048760/3607358 or

I use this also for my tests in another project.
I could add this in the next days if it is wanted.

Yeah, this is how I did it for one project. I think it would be good to implement this so that, there is no need to maintain golden files for every OS.

@imaNNeo 93% sounds quite high.

Maybe the new Flutter version (3.16.0) gives that 93%?

I'm not sure if I ran it on 3.13.6 before 🤔

@imaNNeo
Copy link
Owner

imaNNeo commented Nov 24, 2023

No worries @bobatsar

Yes, I was in the latest version (3.16.0).
And about the OS, I think one OS might be enough with a tolerance.

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.

3 participants