Skip to content

Conversation

@madscientist159
Copy link
Contributor

This enables proper rendering of horizontal bar charts in stacked mode. This is functionally identical to the vertical mode.

@derickr
Copy link
Member

derickr commented Dec 11, 2015

This looks reasonable - but I'm not the Graph expert. @kore is. Could you provide a test case however? Something similar to https://github.com/zetacomponents/Graph/blob/master/tests/renderer_2d_test.php#L392..L430?

@madscientist159
Copy link
Contributor Author

Test case added. I'm not sure its coverage is the best, but it's a start at least.

@madscientist159
Copy link
Contributor Author

What else do I need to do to get this merged?

@derickr
Copy link
Member

derickr commented Dec 12, 2015

Not much, I was hoping for @kore to review this... as he's the original author. Let's give him a few days.

@kore
Copy link
Member

kore commented Dec 15, 2015

There seems to be another feature included in this PR, a new fillLine option for radar charts.

I am not aware of the code enough any more to be able to tell if this could have side effects on anything.

Will probably be fine to merge.

@madscientist159
Copy link
Contributor Author

Yeah, I'm not all that familiar with GitHub so I don't really know how to continue to add code to the repository without also adding to this pull request. I'm OK with merging all of this, but if you are not is there a way to just merge the patch we were discussing?

@madscientist159
Copy link
Contributor Author

So, anything else I need to do before merge?

@madscientist159
Copy link
Contributor Author

This has been sitting for a while...anything else that needs to be done?

@derickr
Copy link
Member

derickr commented Jan 4, 2016

Sorry - holiday season and all that! From what I understand, there are three commits for all three different features? It only looks like the first one has a test case? It's probably better to add a test case for each commit as well, and then split this up into three pull requests (although, I guess, that's a bit of an overkill). But having a test case for each feature, is going to be sort of a requirement. The code looks good though!

@madscientist159
Copy link
Contributor Author

EDIT: Needed to refresh to see your post.

Is there a way to just pull in the first commit on GitHub or do I need to fork multiple copies of the Zeta Components repository to break up the pull request?

@derickr
Copy link
Member

derickr commented Jan 7, 2016

I think your best bet is to do the following (crossing fingers I didn't make typoes!)

  1. Write down the three commit hashes: 825295b (stacked rendering), ac2c7e0 (specific dataset) and a233c63 (boundfill)
  2. reset your master branch to the remote one: git checkout master, git fetch origin, git reset --hard origin/master2. create a new branch for the original new feature: git checkout master, git checkout -b stacked-horizontal-barchart and add the commit: git cherry-pick 825295b8ae3979aff902f485edf00e8e513fb067
  3. reset your master branch to the remote one (only if all the above worked without errors!): git checkout master, git fetch origin, git reset --hard origin/master
  4. create a new branch for the specific dataset feature: git checkout master, git checkout -b render-specific-dataset and add the commit: git cherry-pick ac2c7e05d104d1dc39ccf5391b13103dab7d251f
  5. create a new branch for the bound fill feature: git checkout master, git checkout -b boundfill and add the commit: git cherry-pick a233c63b7379fe04219b5d666877e71dec3ba9e0
  6. push all these branches to your personal repository: git checkout master, git push origin master stacked-horizontal-barchart render-specific-dataset boundfill
  7. If you now go to https://github.com/madscientist159/Graph you should see that there are "three branches" that you can create a PR from. Go ahead and click the button for it (3 times).
  8. Locally, you can now switch between your three feature branches with git checkout <branchname>. For example: git checkout boundfill.
  9. Then add a test case, and when you have done so, run: git add ...files..., git commit, git push origin <branchname> (git push origin boundfill). This adds the commit to the end of the PR, and you don't need to create a pull request for it.
  10. Do the same for the stacked-horizontal-barchart branch.

In general, you should never commit anything to master, but always make a branch - for both features and bug fixes. That way, you can organise your work much more easily, and not have multiple features/fixes in one pull request.

If you have any issues wrangling GIT, feel free to join us on IRC on FreeNode/#zetacomponents. I'm in there as Derick.

@madscientist159
Copy link
Contributor Author

Understood. I know how to get GIT to do what is desired, I just didn't realize each PR on GitHub needed to be from a separate branch.

I'll try to get to this in the next few days.

@madscientist159
Copy link
Contributor Author

Continued in #19

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