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: Range Annotations #167

Merged
merged 22 commits into from
Jan 17, 2020
Merged

Conversation

shamilovtim
Copy link

@shamilovtim shamilovtim commented Jan 15, 2020

This PR adds range annotations to line charts. Docs are updated in the linechart document.

Screen Shot 2020-01-15 at 4 49 14 PM

To use:

  rangeAnnotations: RangeAnnotations(
        hasVerticalRangeAnnotations: true,
        verticalRangeAnnotations: [
          VerticalRangeAnnotation(
            x1: 2,
            x2: 4,
            color: Colors.orange
          ),
          VerticalRangeAnnotation(
            x1: 8,
            x2: 10,
            color: Colors.yellow
          ),
        ],
        hasHorizontalRangeAnnotations: true,
        horizontalRangeAnnotations: [
          HorizontalRangeAnnotation(
            y1: 1,
            y2: 2,
            color: Colors.red
          ),
          HorizontalRangeAnnotation(
            y1: 4,
            y2: 5,
            color: Colors.blue
          ),
        ]
      ),

@shamilovtim
Copy link
Author

Closes #163

@imaNNeo
Copy link
Owner

imaNNeo commented Jan 16, 2020

Everything looks good and thank you for your great pull request, but there is one thing left,
please make a nice sample, and please don't add it in the first page, don't add anything in pages 1 to 4, you can add it in page 5 (line_chart_page2.dart)

@shamilovtim
Copy link
Author

I have a suggestion about it:

if (data.extraLinesData != null && !data.extraLinesData.extraLinesOnTop) {
   drawExtraLines(canvas, size);
}

drawBarLine(canvas, size, barData);
drawDots(canvas, size, barData);
drawTouchedSpotsIndicator(canvas, size, barData);

if (data.extraLinesData != null && data.extraLinesData.extraLinesOnTop) {
   drawExtraLines(canvas, size);
}

Does it make sense?

Yes! It works. Just one thing:

drawTouchedSpotsIndicator(canvas, size, barData); should come last? I believe this part of the order might been a bug. Otherwise it looks visibly broken when the touch indicator is covered up by a chart line.

@shamilovtim
Copy link
Author

data.extraLinesData != null not necessary we have an empty array there due to default value and no matter what it's never null.

@imaNNeo
Copy link
Owner

imaNNeo commented Jan 16, 2020

I have a suggestion about it:

if (data.extraLinesData != null && !data.extraLinesData.extraLinesOnTop) {
   drawExtraLines(canvas, size);
}

drawBarLine(canvas, size, barData);
drawDots(canvas, size, barData);
drawTouchedSpotsIndicator(canvas, size, barData);

if (data.extraLinesData != null && data.extraLinesData.extraLinesOnTop) {
   drawExtraLines(canvas, size);
}

Does it make sense?

Yes! It works. Just one thing:

drawTouchedSpotsIndicator(canvas, size, barData); should come last? I believe this part of the order might been a bug. Otherwise it looks visibly broken when the touch indicator is covered up by a chart line.

You're right,
drawTouchedSpotsIndicator(canvas, size, barData); should be after all things.

@imaNNeo
Copy link
Owner

imaNNeo commented Jan 16, 2020

data.extraLinesData != null not necessary we have an empty array there due to default value and no matter what it's never null.

we are using data.extraLinesData.extraLinesOnTop in our if statement, and for sure if data.extraLinesData is null, app crashes due to NullPointerException (this is before our draw function).

@shamilovtim
Copy link
Author

data.extraLinesData can't be null though? I confirmed this with the debugger. it's initialized with an empty array at all times.

@imaNNeo
Copy link
Owner

imaNNeo commented Jan 16, 2020

extraLinesData

What about if I have this code?

LineChartData(
   extraLinesData: null,
)

@shamilovtim
Copy link
Author

ok almost done

@shamilovtim
Copy link
Author

all set

padding inside the listview was throwing off the touch indicator
@imaNNeo
Copy link
Owner

imaNNeo commented Jan 17, 2020

Hi man, everything is well, and it's ready to merge,
but there is a last request, please make the sample more smooth, I think we just need to have some colors for annotations with opacity, current colors are too sharp,
and add this sample screenshot in the line_chart.md,
with the title: Sample 8 - range annotations

@shamilovtim
Copy link
Author

used analogous colors

Screen Shot 2020-01-17 at 1 42 01 PM

@shamilovtim
Copy link
Author

that should be it

@imaNNeo
Copy link
Owner

imaNNeo commented Jan 17, 2020

Great!

@imaNNeo
Copy link
Owner

imaNNeo commented Jan 17, 2020

Everything looks good and thank you for your great pull request, but there is one thing left,
please make a nice sample, and please don't add it in the first page, don't add anything in pages 1 to 4, you can add it in page 5 (line_chart_page2.dart)

move your sample to the line_chart_page2.dart, please

@shamilovtim
Copy link
Author

@imaNNeo
Copy link
Owner

imaNNeo commented Jan 17, 2020

Okay, apologize.

@imaNNeo imaNNeo merged commit 7446f0d into imaNNeo:master Jan 17, 2020
@imaNNeo
Copy link
Owner

imaNNeo commented Jan 17, 2020

Thanks for your great PR,
hope to see your more PRs.
Cheers!

@shamilovtim shamilovtim deleted the range-annotations branch January 17, 2020 20:44
@shamilovtim shamilovtim restored the range-annotations branch January 17, 2020 20:46
@shamilovtim shamilovtim deleted the range-annotations branch January 17, 2020 20:46
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