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

tcl.minor default #460

Merged
merged 7 commits into from
Mar 22, 2017
Merged

Conversation

jordansread
Copy link
Member

for #437

I ended up doing this in a way that isn't testable except for the result of the render...

This is because I don't want the fraction of par('tcl') to be calculated until it is relevant - i.e., when the side is being rendered, so it is stuck into the draw_axis w/ the arg = NA signifying to use this default. It is buried from the user, but the alternative solution that covers this and was discussed in #437 with the hierarchy is too complex for the value of this functionality. Additional thoughts welcome.

@lindsayplatt
Copy link

Since it's not testable except for rendering, did you build any of the vignettes? I imagine we would see a change because the tcl.minor default was 0.15 and should now be -0.25 if par('tcl') remains unchanged.

I think having NA carry through to the final gsplot object makes sense to me. People can just look in the documentation to figure out what the actual value should be since it could change if pars change.

Copy link

@lindsayplatt lindsayplatt left a comment

Choose a reason for hiding this comment

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

Change the gsplot config to have a default tcl=0.5 instead of the default par which is tcl=-0.5, so that ticks remain on the inside.
Build the vignettes and README first.

@jordansread
Copy link
Member Author

@lindsaycarr the tcl is in the default config already as 0.3. I will keep that as-is. Working on merge conflicts for the vignettes now and that is coming w/ the next commit.

Merge branch 'master' of github.com:USGS-R/gsplot into tcl_minor_default

# Conflicts:
#	inst/doc/gsplotIntro.html
@jordansread
Copy link
Member Author

changes in the vignettes all seem to be due to the retina screen render. Note we don't have anything in the readme that calls out the tcl.minor, so we don't see changes there.

@lindsayplatt
Copy link

Doesn't it appear that the new fig changes are overplotting axes? Or is that just a visual trick?

@jordansread
Copy link
Member Author

Hard to say if overplotting or retina. I didn't change anything related to append vs replace for axis, so not sure why this would overplot if it didn't before this commit.

@jordansread
Copy link
Member Author

I added a change to the readme to force it to use the tcl.minor default @lindsaycarr

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 0aef465 on jread-usgs:tcl_minor_default into ** on USGS-R:master**.

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