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 rotate angle to chart #124

Merged
merged 8 commits into from
Dec 11, 2019
Merged

Add rotate angle to chart #124

merged 8 commits into from
Dec 11, 2019

Conversation

arefhosseini
Copy link
Contributor

add rotateAngle parameter to SideTitles.
consider to this issue I just add this feature to the chart.

Copy link
Owner

@imaNNeo imaNNeo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, I appreciate your work, thank you man.

move this param to line chart sample 5
@arefhosseini
Copy link
Contributor Author

Hey there, fix your notes. I added rotateAngle to line chart sample 5, Hope its fine.
Much an appreciate.

add alignment for text side titles
add alignment for text side titles
@arefhosseini
Copy link
Contributor Author

Add SideTitlesAlignment to SideTitles for better performance in rotating & positioning.
There is and bug when rotating specially in 90 degrees, and I just add this parameter to SideTitles for better rendering.
note: would better use SideTitlesAlignment.end for 90 degrees in rotateAngle.
Hope its handy for you & notify me its ok or not.

@imaNNeo
Copy link
Owner

imaNNeo commented Dec 5, 2019

Hey there, good catch about the bug,
still, there are some states that alignment is not handled,
for example, this is rotated in 180 degrees with centered alignment:
image

please think about it and introduce a way to handle all kinds of these states for all top, right, bottom, left sides.
Maybe we can handle it without alignment (calculate it with the degree, I'm not sure)
Let me know what you think.

@arefhosseini
Copy link
Contributor Author

Good point, didn't think about that at all. Will fix these states.

@arefhosseini
Copy link
Contributor Author

Fix rotating and translate of center TextPainter

Screenshot_1575844003

@imaNNeo
Copy link
Owner

imaNNeo commented Dec 9, 2019

Looks great man :)

@arefhosseini
Copy link
Contributor Author

Thanks

@imaNNeo
Copy link
Owner

imaNNeo commented Dec 10, 2019

Everything looks great,
I've checked it and working like a charm.
I appreciate this feature. I like it.
Waiting for your replay.
Thanks!

@arefhosseini
Copy link
Contributor Author

Your welcome & thanks to you for cooperating & I'm glad it works.
If you think I should add some notes in README or something like this, will do.

@imaNNeo
Copy link
Owner

imaNNeo commented Dec 11, 2019

Did you read my review comment?
About the ReadMe, yes it's gonna be great if you help me,
1 -> Please update the modified BarChartSample5 (just set the background of screen white then take a snapshot and crop it, and replace with ./repo_files/bar_chart/bar_chart_sample_5.png)
2 -> Please update the SideTitles (add new properties with their description)
That's it, then we are ready to merge.
Thanks!

change snapshot of bar chart sample 5
@arefhosseini
Copy link
Contributor Author

All set

@@ -111,6 +111,13 @@ class FlTitlesData {
}
}

/// this is mimic of [MainAxisAlignment] to aligning the [SideTitles]
enum SideTitlesAlignment {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we don't need SideTitlesAlignment anymore?
Can you explain it's usage?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default position of TextPainter is center of it's x-axis point.
I just add this alignment for whom wanna set this view for better rendering in special cases.
If you think it's an irrelevant parameter I can remove it.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please let's remove it because it is not fully customizable, we can just customize the x-axis pivot, and for sure we should add customization for both axes.
We can add it in the future if it is needed.
Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's a good point, I just removed it.

@imaNNeo
Copy link
Owner

imaNNeo commented Dec 11, 2019

Great, thank you for being patient and resolve all discussions.
Hope to see your more Pull Requests, I really appreciate and liked this one.
This feature will be in the next version.
Thanks!

@imaNNeo imaNNeo merged commit 79ed9a5 into imaNNeo:master Dec 11, 2019
@imaNNeo
Copy link
Owner

imaNNeo commented Dec 11, 2019

image

Oh man, this should be white backgrounded (currently it is gray).
please fix it in another PR.
Thanks :)

@imaNNeo
Copy link
Owner

imaNNeo commented Dec 12, 2019

Forget about it, I've just fixed it, see in the commit.

@arefhosseini
Copy link
Contributor Author

Much an apprentice buddy. So glad to work with you

@imaNNeo
Copy link
Owner

imaNNeo commented Dec 12, 2019

Hope to see your more PRs :)
Cheers!

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.

2 participants