Vertical text for FLineLabel

imaNNeo / fl_chart

FL Chart is a highly customizable Flutter chart library that supports Line Chart, Bar Chart, Pie Chart, Scatter Chart, and Radar Chart.

https://flchart.dev

MIT License

6.87k stars 1.78k forks source link

Vertical text for FLineLabel #1579

Closed julien4215 closed 7 months ago

julien4215 commented 9 months ago

Closes #1574.

The goal of this PR is give the option to display the text of the label vertically for the VerticalLineLabel and HorizontalLineLabel (see the screenshot in the issue #1574).

codecov[bot] commented 9 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 87.51%. Comparing base (5a01893) to head (d1041ff). Report is 1 commits behind head on main.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #1579 +/- ## ========================================== + Coverage 86.38% 87.51% +1.13% ========================================== Files 45 45 Lines 3047 3075 +28 ========================================== + Hits 2632 2691 +59 + Misses 415 384 -31 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

imaNNeo commented 9 months ago

With alignment: Alignment.topLeft and direction: LabelDirection.vertical, it draws outside of the chart

But with alignment: Alignment.topRight and direction: LabelDirection.vertical it draws inside of the chart. So I think we should draw it inside in either ways.

imaNNeo commented 9 months ago

One question, does it make sense to have angle instead of limited direction values? With angle, I think we are free to do anything. But I agree that it is more difficult and challenging to implement.

imaNNeo commented 9 months ago

Please write some unit-tests

julien4215 commented 9 months ago

One question, does it make sense to have angle instead of limited direction values? With angle, I think we are free to do anything. But I agree that it is more difficult and challenging to implement.

It makes sense to have an angle parameter as the label could be displayed in each direction, however it would require a lot of effort for really rare use cases.

imaNNeo commented 9 months ago

One question, does it make sense to have angle instead of limited direction values? With angle, I think we are free to do anything. But I agree that it is more difficult and challenging to implement.

It makes sense to have an angle parameter as the label could be displayed in each direction, however it would require a lot of effort for really rare use cases.

You're right. Okay so let's stick to the current implementation and write some unit-tests please

julien4215 commented 9 months ago

Why are Mockito generated files not ignored ? Should I commit them ?

julien4215 commented 9 months ago

Please write some unit-tests

Done

julien4215 commented 9 months ago

With alignment: Alignment.topLeft and direction: LabelDirection.vertical, it draws outside of the chart

But with alignment: Alignment.topRight and direction: LabelDirection.vertical it draws inside of the chart. So I think we should draw it inside in either ways.

This behavior is the same for VerticalLineLabel with direction: LabelDirection.horizontal so it makes sense for me to keep the same logic.

With alignment: Alignment.bottomLeft and direction: LabelDirection.horizontal`, it draws outside of the chart

With alignment: Alignment.topLeft and direction: LabelDirection.horizontal`, it draws inside of the chart

imaNNeo commented 9 months ago

With alignment: Alignment.topLeft and direction: LabelDirection.vertical, it draws outside of the chart But with alignment: Alignment.topRight and direction: LabelDirection.vertical it draws inside of the chart. So I think we should draw it inside in either ways.

This behavior is the same for VerticalLineLabel with direction: LabelDirection.horizontal so it makes sense for me to keep the same logic.

With alignment: Alignment.bottomLeft and direction: LabelDirection.horizontal`, it draws outside of the chart

With alignment: Alignment.topLeft and direction: LabelDirection.horizontal`, it draws inside of the chart

I think the vertical one is a bug too. I mean we need to fix both of them. It would be nice if you can fix them here, otherwise I will do this in a separate PR. What do you think?

julien4215 commented 9 months ago

I think the vertical one is a bug too. I mean we need to fix both of them. It would be nice if you can fix them here, otherwise I will do this in a separate PR. What do you think ?

I agree that it would be better like this. I'll add a commit to this PR for both of them.

imaNNeo commented 9 months ago

I agree that it would be better like this. I'll add a commit to this PR for both of them.

That would be wonderful

julien4215 commented 9 months ago

I agree that it would be better like this. I'll add a commit to this PR for both of them.

Done, it should be ready now.

imaNNeo commented 8 months ago

It's ready to be merged after those minor changes. Thanks in advance!