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

Feature/line chart extra lines #18

Closed
wants to merge 5 commits into from
Closed

Feature/line chart extra lines #18

wants to merge 5 commits into from

Conversation

agustka
Copy link

@agustka agustka commented Jun 11, 2019

Adding extra line data to line charts that contains horizontal and vertical lines.

@@ -129,6 +131,9 @@ class LineChartBarData {
/// to show dot spots upon the line chart
final FlDotData dotData;

// to show line chart annotations such as average line and vertical dot lines
final ExtraLinesData extraLinesData;
Copy link
Owner

Choose a reason for hiding this comment

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

Here we don't need it, because ExtraLinesData as defined on LineChartData, we should use that to draw lines in the whole chart, and not related any LineChartBarData,
Please remove it, and for the below lines as I said we should put it on the BelowBarData, I will explain it on the next sections.

Copy link
Owner

Choose a reason for hiding this comment

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

As I saw you defined it to the LineChartData

Copy link
Author

Choose a reason for hiding this comment

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

Removed.

}

class DashDefinition {
Copy link
Owner

Choose a reason for hiding this comment

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

Please remove it, we can add line style functionality line Dash and Dot effects in next phase, here we just add two feature (Extra Lines -> you use it for avg line, and BelowBar Lines-> you use it for below bar lines), then I will add LineStyle after this PR, don't worry about that.

Copy link
Author

Choose a reason for hiding this comment

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

All right, although I will not be able to use fl_chart library without it.

Copy link
Owner

Choose a reason for hiding this comment

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

We can not add all features in a single PR, I said multiple times that I will add it after your pR, it should add in a separate phase, because I want to reuse it on other parts such as grid and borders, this is a public library and we should implement all features generic and customizable, (for example add LineStyle on other parts).
be patient

class VerticalLine extends LineChartLine {
final double x;
final double endY;
Copy link
Owner

Choose a reason for hiding this comment

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

we don't need endY because ExtraLines should be drawn from y = 0 to y = height, and the only x is dynamic ( for the below line we should apply another solution), prefer this ExtraLines just for avg line.

Copy link
Author

@agustka agustka Jun 11, 2019

Choose a reason for hiding this comment

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

I need the line to end at a datapoint, not at the top of the chart...

Copy link
Owner

Choose a reason for hiding this comment

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

If we want to draw this to the specific data point then we should have start data point alongside it.
I said, use it just for full horizontal and vertical lines, and it can be used as a avg line in your usecase, then you should use BelowBarData to draw lines below dot points

Copy link
Author

Choose a reason for hiding this comment

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

After checking it some more I understand better what below bar data is for, so I'm dropping endY.

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.

For the lines below the spots, you can define it on the BelowBarData, because it is related to the bar, then we don't need to get any endX and endY from the user because we have the Spot x and y, then ve can draw a vertical line below it.
implement the data class like this:

class BelowBarData {
 …
 FlLine lineBelowSpot; // this is a line that determines color and strokeWidth of the line and draw from the spot’s x and minimum y position to the spot’s x and y.
 …
}

class HorizontalLine extends LineChartLine {
final double y;
final double endX;
Copy link
Owner

Choose a reason for hiding this comment

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

also we don't need endX, (read previous comment)

Copy link
Author

@agustka agustka Jun 11, 2019

Choose a reason for hiding this comment

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

What if the user wants the line to only go to a certain data point though and not necessarily to the end of the chart...? I added it for consistency with VerticalLine but I can easily drop it.

Copy link
Owner

Choose a reason for hiding this comment

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

Drop it plz, if we want to make it possible to set end point then we have to add start point for it to be customizable

Copy link
Author

Choose a reason for hiding this comment

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

Dropped

@@ -40,6 +45,8 @@ class LineChartPainter extends AxisChartPainter {
});

drawTitles(canvas, viewSize);

drawAnnotation(canvas, viewSize);
Copy link
Owner

Choose a reason for hiding this comment

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

dude, this is not annotation in a generic definition, look at the problem as a generic problem, we should name it extraLines to draw extra lines on the chart, then you can use it for your problem to draw avg line, this is not related to the FL Chart.

Copy link
Author

Choose a reason for hiding this comment

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

Residual naming from the original work when I first started. Renaming.

viewSize = getChartUsableDrawSize(viewSize);
extraLinesPaint.color = line.color;
extraLinesPaint.strokeWidth = line.strokeWidth;
final double x = line.endX == null ? viewSize.width : getPixelX(line.endX, viewSize);
Copy link
Owner

Choose a reason for hiding this comment

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

here we can equal x with the viewSize.width every time.
because this is not for draw below lines

Copy link
Author

Choose a reason for hiding this comment

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

Removed

if (line.dashDefinition != null) {
_drawDashedLine(canvas, extraLinesPaint, 0, y, x, y, line.dashDefinition);
} else {
_drawSolidLine(canvas, extraLinesPaint, 0, y, x, y);
Copy link
Owner

Choose a reason for hiding this comment

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

remove this if else and determine the line as a solid line every time ( I will add Line Style after your PR in a separate phase), and you can draw the solid line here and you don't need to put it in a new function (because there is no if else anymore)

Copy link
Author

Choose a reason for hiding this comment

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

Removed

@@ -162,6 +164,9 @@ class BelowBarData {
final Offset gradientFrom;
final Offset gradientTo;


final List<VerticalLine> verticalLines;
Copy link
Owner

Choose a reason for hiding this comment

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

here we should just have a single FlLine that determines the style of line, then we can repeat it for all spot points, we don't need VerticalLine, because we know where we should draw them (below the spots)

Copy link
Author

Choose a reason for hiding this comment

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

Dropped in favor of FlLine


class HorizontalLine extends FlLine {
final double y;
Copy link
Owner

Choose a reason for hiding this comment

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

We have a convention on Library,
the horizontal is all of the things that draw on the x axis,
the vertical is all of the things that draw on the y axis,
then HorizontalLine means lines that draw on the x axis and ( for example in x = 1, 2, 3, 4, 5), and y is the same (may be it is VerticalLine in your head, but it is a convention in this library, check the FlTitlesData you can find out what does it means.)
conclusion:
we should have x here instead of y

Copy link
Author

@agustka agustka Jun 12, 2019

Choose a reason for hiding this comment

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

Won't this confuse everybody? A horizontal line has infinite x values... seems strange but I'll change it.

Copy link
Author

Choose a reason for hiding this comment

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

Y has been changed to x.

@@ -326,4 +348,31 @@ class LineChartPainter extends AxisChartPainter {

@override
bool shouldRepaint(CustomPainter oldDelegate) => false;

void drawExtraLines(Canvas canvas, Size viewSize) {
Copy link
Owner

Choose a reason for hiding this comment

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

Here plz draw both horizontal and vertical lines, (firstly data class should be updated)

Copy link
Author

Choose a reason for hiding this comment

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

The drawing of the two line types has been merged into this method.

Copy link
Author

Choose a reason for hiding this comment

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

But I'm not sure what you mean with that the data class should be updated?

}

/// This class holds data about drawing chart annotations (data decorations) such as average line and data point lines
Copy link
Owner

Choose a reason for hiding this comment

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

Update the comment, ( we don't need to mention anything about annotation, this word is specific for your use case)

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

@@ -317,6 +323,36 @@ class LineChartPainter extends AxisChartPainter {
}
}

void drawExtraLines(Canvas canvas, Size viewSize) {
// Draw vertical data point lines under spots
Copy link
Owner

Choose a reason for hiding this comment

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

her is not under spots, fix the comment

// Draw vertical data point lines under spots
viewSize = getChartUsableDrawSize(viewSize);
for (LineChartBarData barData in data.lineBarsData) {
if (barData.belowBarData.verticalLines == null) {
Copy link
Owner

Choose a reason for hiding this comment

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

why you are reading it from belowBarData?
dude draw it just like the horizontal lines,
you should have a class like this:

class ExtraLinesData {
  final bool show;

   // Horizontal line definitions
  final List<HorizontalLine> horizontalLines;

  // Vertical line definitions
  final List<VerticalLine> verticalLines;

   const ExtraLinesData({
    this.show = true,
    this.horizontalLines,
    this.verticalLines,
  });
}

both of them should define in this class

Copy link
Author

Choose a reason for hiding this comment

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

You told me to move it to below bar data...

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