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

Add the ability to rotate the RadarChart titles #1057

Merged
merged 1 commit into from
Jun 17, 2022

Conversation

FlorianArnould
Copy link
Contributor

Just one question :
Do we need to update the Markdown radar chart doc with more detailed information about the need behavior of the RadarChartData.getTitle field
Maybe the migration doc too

@codecov
Copy link

codecov bot commented Jun 11, 2022

Codecov Report

Merging #1057 (1c479f6) into master (1430cd9) will increase coverage by 0.08%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1057      +/-   ##
==========================================
+ Coverage   86.24%   86.32%   +0.08%     
==========================================
  Files          45       45              
  Lines        2777     2808      +31     
==========================================
+ Hits         2395     2424      +29     
- Misses        382      384       +2     
Impacted Files Coverage Δ
lib/src/chart/radar_chart/radar_chart_painter.dart 100.00% <100.00%> (ø)
...src/chart/scatter_chart/scatter_chart_painter.dart 93.67% <0.00%> (-0.91%) ⬇️
lib/src/chart/bar_chart/bar_chart_painter.dart 94.13% <0.00%> (+0.16%) ⬆️
lib/src/chart/line_chart/line_chart_painter.dart 83.58% <0.00%> (+0.22%) ⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@imaNNeo
Copy link
Owner

imaNNeo commented Jun 16, 2022

Everything is good and Approved.

But before merging please do the below requirements.

  1. Please update the radar_chart.md doc to add RadarChartTitle section.

  2. Please write about your BREAKING change in the CHANGELOG.md fil. Also, it would be great if you write a migration guide (something like what we did for 0.50.0 version here. You can name it MIGRATION_NEW_VERSION.md for now. I will rename it whenever I created our next version.

Thanks in advance!

@FlorianArnould FlorianArnould force-pushed the feature/radar-rotate-titles branch 2 times, most recently from ae585ac to c278d96 Compare June 16, 2022 20:53
@FlorianArnould
Copy link
Contributor Author

Ok, I added some doc as you asked I'm not sure if it's enough or too much.
Don't hesitate if there is something else to change, add or delete 😉

CHANGELOG.md Outdated Show resolved Hide resolved
@imaNNeo
Copy link
Owner

imaNNeo commented Jun 16, 2022

I think it's better to move your images beside the migration file. Because they are fully related to the migration file (in images/*_chart/ we put images that have a corresponding implementation in our example app).
What do you think?

@FlorianArnould
Copy link
Contributor Author

FlorianArnould commented Jun 16, 2022

I think it's better to move your images beside the migration file. Because they are fully related to the migration file (in images/*_chart/ we put images that have a corresponding implementation in our example app). What do you think?

Yes seems better to me to avoid flooding the example image folder. I moving all of this.

@imaNNeo
Copy link
Owner

imaNNeo commented Jun 17, 2022

Thank you!

@imaNNeo imaNNeo merged commit d530486 into imaNNeo:master Jun 17, 2022
@imaNNeo
Copy link
Owner

imaNNeo commented Jun 17, 2022

It was a straightforward PR.
It's my pleasure to see more from you in this package :)

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.

None yet

2 participants