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.

Sign up for GitHub

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

Jump to bottom

Allow to reverse the order of colours in a brewer palette #36

Merged
merged 2 commits into from
Jun 12, 2015
Merged

Allow to reverse the order of colours in a brewer palette #36

merged 2 commits into from
Jun 12, 2015

Conversation

jiho
Copy link
Contributor

@jiho jiho commented May 16, 2013

Use the same argument that hue_pal uses: direction

@jiho jiho mentioned this pull request May 16, 2013
Closed
@jiho jiho mentioned this pull request Mar 21, 2014
Closed
@hadley
Copy link
Member

hadley commented Jun 10, 2015

Could you please merge/rebase and add a bullet to NEWS?

@jiho
Allow to reverse the order of colours in a brewer palette
20161ad 
Use the same argument name as hue_pal: direction
@jiho
Copy link
Contributor Author

jiho commented Jun 11, 2015

Done, and squashed everything into one commit.

@jiho
Copy link
Contributor Author

jiho commented Jun 12, 2015

I forgot to write the documentation before. Sorry. check() still does not pass on my machine because of problems in creating the PDF documentation. However there is no more info and I don't really know what to fix. Hopefully that's a glitch of my machine and it'll pass on Travis.

@jiho
Copy link
Contributor Author

jiho commented Jun 12, 2015

It passed! It'll be great if you can merge it before the release so that I can depend on it in ggplot2.

hadley
hadley reviewed Jun 12, 2015
View reviewed changes
R/pal-brewer.r
if (direction == -1)
pal <- rev(pal)

return(pal)
Copy link
Member

Choose a reason for hiding this comment

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

For future reference, I prefer to use return() only for early returns

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. I thought it was good practice to always have an explicit return()(it's certainly less confusing for newcomers/students in my experience).

hadley added a commit that referenced this pull request Jun 12, 2015
@hadley
Merge pull request #36 from jiho/feature/reverse_brewer
6729f36 
Allow to reverse the order of colours in a brewer palette
@hadley hadley merged commit 6729f36 into r-lib:master Jun 12, 2015
@hadley
Copy link
Member

hadley commented Jun 12, 2015

Thanks!

@hadley
Copy link
Member

hadley commented Jun 12, 2015

BTW if you have a little more time, I'd love a PR with some tests :)

@jiho
Copy link
Contributor Author

jiho commented Jun 12, 2015

What would you want me to test in this particular instance? The order of colors returned? The values of colors returned?

@jiho
Copy link
Contributor Author

jiho commented Jun 12, 2015

Thanks for merging it anyway, I'll get to work on the ggplot end now!

@jiho jiho deleted the feature/reverse_brewer branch June 18, 2015 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jiho @hadley