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

Issues #182, #122 #183

Closed
wants to merge 2 commits into from
Closed

Issues #182, #122 #183

wants to merge 2 commits into from

Conversation

dorsoft
Copy link

@dorsoft dorsoft commented Jul 1, 2015

#182 ChartData.removeEntryByXIndex removes the wrong entry
#122 Candle chart - make the shadow same color as an increasing/decreasing candle color

@danielgindi
Copy link
Collaborator

Hey man, thanks!
But:

  1. You should separate those into two PRs (Just create two branches based on the latest commit, and cherry-pick the commits into them)
  2. Please make sure code styling matches (That means { and } are on new lines, no ; in line endings in Swift)
  3. For shadow colors, we are working on a different solution, possibly more flexible, so it's still pending :-)

@liuxuan30
Copy link
Member

Hi @danielgindi, I am also new to submit different PRs.. Any wiki for this? what you mean by cherry-pick the commits into them?
What I have is only a master branch in my fork, and whenever I push to my origin master, github automatically put it into the same PR, though it is for different bug...

@danielgindi
Copy link
Collaborator

Well as I said, commit to different branches.
You can make a PR for each branch of yours.
https://help.github.com/articles/using-pull-requests/

@liuxuan30
Copy link
Member

Thanks. I will follow this rule next time. For my previous PR #168, sorry I have merged several fixes together. most of them are related to the same issue.

@danielgindi
Copy link
Collaborator

Still, you need to just create branches, from the master commit before your
changes.
Then take your new commits and move to those branches

On Thu, Jul 2, 2015 at 8:29 AM, Xuan notifications@github.com wrote:

Thanks. I will follow this rule next time. For my previous PR #168
#168, sorry I have merged
several fixes together. most of them are related to the same issue.


Reply to this email directly or view it on GitHub
#183 (comment)
.

@liuxuan30
Copy link
Member

Maybe we could talk about it in PR #168? This PR is old, and I have been keeping it synced with the latest code. So it can be automatically merged.

@danielgindi danielgindi closed this Jul 9, 2015
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