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

added axis titles #115

Merged
merged 6 commits into from
Dec 30, 2019
Merged

added axis titles #115

merged 6 commits into from
Dec 30, 2019

Conversation

j4nk3e
Copy link
Contributor

@j4nk3e j4nk3e commented Nov 19, 2019

I implemented the possibility to add axis descriptions to the line charts.
You can add text to each axis on the left, right, top and bottom, which will be shown outside of the current axis titles.

@imaNNeo
Copy link
Owner

imaNNeo commented Nov 27, 2019

Hi there,
any news?

@j4nk3e
Copy link
Contributor Author

j4nk3e commented Nov 27, 2019

Do you need anything to handle this PR?

@imaNNeo
Copy link
Owner

imaNNeo commented Nov 27, 2019

Did you see my comment?

@j4nk3e
Copy link
Contributor Author

j4nk3e commented Nov 27, 2019

0 comments on commit cf18909 nope, sry

@@ -38,7 +38,8 @@ class LineChartSample5 extends StatelessWidget {
return MapEntry(
index,
[
LineBarSpot(tooltipsOnBar, lineBarsData.indexOf(tooltipsOnBar), tooltipsOnBar.spots[index]),
LineBarSpot(tooltipsOnBar, lineBarsData.indexOf(tooltipsOnBar),
Copy link
Owner

Choose a reason for hiding this comment

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

Hi there, really nice to see your PR.
Please prevent to commit these kinds of reformats.
It makes the review hard, and also some redundant changes, please revert all of the changes due to reformat, then I will review it and I will merge it.
Thanks!

@imaNNeo
Copy link
Owner

imaNNeo commented Nov 27, 2019

Sorry man, I didn't submit it.
Check it out.
Thanks!

@j4nk3e
Copy link
Contributor Author

j4nk3e commented Nov 27, 2019

Of course. My AS is configured to run dartfmt on save.
I updated the commit without formatting.

@imaNNeo
Copy link
Owner

imaNNeo commented Nov 30, 2019

Hi @juumixx,
I've just reviewed your code and everything looks good, I appreciate it, it is perfect.
May I ask you to implement it in BarChart and ScatterChart too, for the sake of consistency?

@imaNNeo
Copy link
Owner

imaNNeo commented Nov 30, 2019

also please rebase your branch to retrieve the ScatterChart.

@imaNNeo
Copy link
Owner

imaNNeo commented Dec 5, 2019

Hello??

@j4nk3e
Copy link
Contributor Author

j4nk3e commented Dec 6, 2019

Hi, sorry I didn't have the time yet to look into the other charts. I will try to build it into the other charts, too.

@imaNNeo
Copy link
Owner

imaNNeo commented Dec 6, 2019

Thanks man!
Waiting for you.

@j4nk3e
Copy link
Contributor Author

j4nk3e commented Dec 10, 2019

I updated my branch and am working on integrating the axis titles to the bar charts now.

@imaNNeo
Copy link
Owner

imaNNeo commented Dec 10, 2019

Thanks!

@j4nk3e
Copy link
Contributor Author

j4nk3e commented Dec 10, 2019

Actually, I'm moving it up to the AxisChartPainter, so it should also work for the ScatterChart

@j4nk3e
Copy link
Contributor Author

j4nk3e commented Dec 10, 2019

I still added an example to the scatter charts. I think it is ready to merge now.

@imaNNeo
Copy link
Owner

imaNNeo commented Dec 10, 2019

Thanks, man, I'm gonna review it.

@imaNNeo
Copy link
Owner

imaNNeo commented Dec 10, 2019

And again thank you for this great feature.
I've tested it and it is working, I appreciate it.
Waiting for your changes, I promise I will merge it ASAP.
Thanks!

@j4nk3e
Copy link
Contributor Author

j4nk3e commented Dec 11, 2019

Did you leave comments somewhere in the code?

@imaNNeo
Copy link
Owner

imaNNeo commented Dec 11, 2019

Yes but I forgot to submit them, sorry about that.

@imaNNeo
Copy link
Owner

imaNNeo commented Dec 14, 2019

Hello, how's it going?
I'm waiting for you to resolve these comments then I will merge this wonderful feature.
Thanks!

@j4nk3e
Copy link
Contributor Author

j4nk3e commented Dec 16, 2019

Thanks for the review. We still have to finish a release until Christmas, but maybe I'll get to fixing the issues on Friday.

@imaNNeo
Copy link
Owner

imaNNeo commented Dec 16, 2019

Ok, thanks for the update.

@j4nk3e
Copy link
Contributor Author

j4nk3e commented Dec 20, 2019

I updated the PR.
Do you think it's ok to leave the painter implementation like it is (#115 (comment))?

@imaNNeo
Copy link
Owner

imaNNeo commented Dec 20, 2019

Yes, leave it there.
May I ask you to update this documentation? and update the samples image here?
Thanks, man.

UPDATE:
and also all of Line, Bar, Scatter chart documentations.

@j4nk3e
Copy link
Contributor Author

j4nk3e commented Dec 23, 2019

Will do

@imaNNeo
Copy link
Owner

imaNNeo commented Dec 26, 2019

Hi there,
any news?

@j4nk3e
Copy link
Contributor Author

j4nk3e commented Dec 30, 2019

Voilà!

@imaNNeo imaNNeo merged commit 1cf6723 into imaNNeo:master Dec 30, 2019
@imaNNeo
Copy link
Owner

imaNNeo commented Dec 30, 2019

Yay, congrats!
waiting for your next contributions.
Thanks a lot :)

@j4nk3e
Copy link
Contributor Author

j4nk3e commented Jan 2, 2020

Thanks :D
More is coming soon.

@kaarens93
Copy link

Hi, I am still having trouble with displaying the axis titles. 'axisTitleData' not defined

@j4nk3e j4nk3e deleted the axis-titles branch January 3, 2020 09:41
@j4nk3e
Copy link
Contributor Author

j4nk3e commented Jan 3, 2020

Hi @kaarens93
Can you please open an issue and provide us with example code to reproduce the issue?

@kaarens93
Copy link

@juumixx hi, I just opened an issue. Thank you! I am sure I'm overlooking something small.

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