Skip to content

Improve painter order#175

Merged
imaNNeo merged 7 commits into
imaNNeo:masterfrom
shamilovtim:patch-range-annotations
Feb 7, 2020
Merged

Improve painter order#175
imaNNeo merged 7 commits into
imaNNeo:masterfrom
shamilovtim:patch-range-annotations

Conversation

@shamilovtim

@shamilovtim shamilovtim commented Jan 21, 2020

Copy link
Copy Markdown
Contributor

Improves the order of painting through the following:

  1. rangeAnnotation painting moved outside of the for loop. extraLines removed from the for loop except when extraLinesOnTop is enabled. probably improved performance.
  2. the order of painting is revised to background → range annotations → grid → extralines → barLine → dots → (optional): extralines → touchIndicator

@shamilovtim shamilovtim requested a review from imaNNeo January 21, 2020 00:13
Comment thread lib/src/chart/line_chart/line_chart_painter.dart
@shamilovtim

Copy link
Copy Markdown
Contributor Author

good to go

@shamilovtim shamilovtim requested a review from imaNNeo January 21, 2020 18:40
@shamilovtim

Copy link
Copy Markdown
Contributor Author

I had one more commit. I needed to paint borders by calling super super paint.

Comment thread lib/src/chart/line_chart/line_chart_painter.dart Outdated
Comment thread lib/src/chart/line_chart/line_chart_painter.dart Outdated
@shamilovtim shamilovtim requested a review from imaNNeo February 5, 2020 23:01
Comment thread lib/src/chart/line_chart/line_chart_painter.dart Outdated
@imaNNeo

imaNNeo commented Feb 6, 2020

Copy link
Copy Markdown
Owner

I think I can't push to this PR, because it is in your fork.

@shamilovtim

Copy link
Copy Markdown
Contributor Author

Screen Shot 2020-02-06 at 2 15 13 PM

that shouldn't be the case. you don't have directions below the merge button that let you push to it?

@imaNNeo

imaNNeo commented Feb 6, 2020

Copy link
Copy Markdown
Owner

image

I think I need permission to push on your forked repo.

@shamilovtim

Copy link
Copy Markdown
Contributor Author

I added permissions to my fork but here is what the checkbox says:
Screen Shot 2020-02-06 at 4 56 08 PM

@shamilovtim

Copy link
Copy Markdown
Contributor Author

I think you might have a duplication:
git push shamilovtim patch-range-annotations the name isn't shamilovtim/patch-range-annotations but only "patch-range-annotations"

@shamilovtim

Copy link
Copy Markdown
Contributor Author

also i just noticed that your current branch says 'shamilovtim-patch-range-annotations'. you could try to rename it maybe?

@imaNNeo

imaNNeo commented Feb 6, 2020

Copy link
Copy Markdown
Owner

I'm lost :))))

@imaNNeo

imaNNeo commented Feb 6, 2020

Copy link
Copy Markdown
Owner

wait for a second

@shamilovtim

Copy link
Copy Markdown
Contributor Author

in your terminal you're pushing to my repo named 'shamilovtim' a branch named shamilovtim/patch-range-annotations but your branch is actually named 'shamilovtim-patch-range-annotations' but the real branch name on my repo is only 'patch-range-annotations'

at least i think that's what's going on. i might be wrong. lol

@imaNNeo imaNNeo force-pushed the patch-range-annotations branch from 5e2542c to 3b091ba Compare February 6, 2020 22:18
@imaNNeo

imaNNeo commented Feb 6, 2020

Copy link
Copy Markdown
Owner

That was a hard work :)

  1. I cloned your repo, then I checked out to the patch-range-annotations branch
  2. then I added imaNNeoFighT/fl_chart as a new remote (I called itneo)
  3. then I ran git fetch neo command
  4. cherry-pick that commit from my remote
  5. rebase your branch with neo/master
  6. force pushed to patch-range-annotations branch

@imaNNeo

imaNNeo commented Feb 6, 2020

Copy link
Copy Markdown
Owner

BTW, It's ready to merge in my opinion,
you can have a look at it, and approve my changes to merge.
Thanks!

@shamilovtim

Copy link
Copy Markdown
Contributor Author

I haven't tested manually but it looks fine to merge to me

@imaNNeo imaNNeo merged commit 3e1e6bb into imaNNeo:master Feb 7, 2020
@shamilovtim shamilovtim deleted the patch-range-annotations branch April 1, 2020 20:02
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