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

Move font specification into global variables #23

Merged
merged 3 commits into from Feb 10, 2020
Merged

Conversation

nmpeterson
Copy link
Contributor

Also improved documentation

@nmpeterson
Copy link
Contributor Author

Hold off on merging this for now. I think I've found a solution allowing the specification of a backup font that I will try to add to this.

@nmpeterson
Copy link
Contributor Author

Never mind, the solution I thought I found turned out to be unreliable. :(

Please review the code and merge.

@matthewstern
Copy link
Contributor

This appears to contain both font-fix AND label_last_point related changes. Is this meant to be both? The font-fix aspects look great, but I do have some questions about the label_last_point feature.

@nmpeterson
Copy link
Contributor Author

What questions?

The bulk of the label-last-point functionality got merged into master accidentally a week or two ago because GitHub's desktop app forced me into doing that when I was merging master into label-last-point. The only code unrelated to fixing fonts in this PR is some extra documentation.

@matthewstern
Copy link
Contributor

matthewstern commented Feb 10, 2020

Ah, forgot about that.

Questions about label_last_point mostly pertain to implicitly automating behavior that the user is likely to need pretty much every time this function is added. Of course, this means more work now, and it may be that some of these things are not worth the effort. But here's what comes to mind:

  1. Could it be programmed in such a way that defaults to taking the Y axis value as the label, rather than requiring the user to set the label attribute in top-line aes? Considering that the following two code blocks yield the same output, can geom_text_lastonly() be set up to inherit the master aes y value as it's label value?

Label in topline aes:

ggplot(grp_over_time, aes(x = year, y = realgrp, color = cluster, label = realgrp)) +
    scale_y_continuous("Gross regional product (indexed to 2007)", labels=scales::percent) +
    scale_x_continuous("Year") +
    geom_line() + geom_text_lastonly()

label in secondary aes:

ggplot(grp_over_time, aes(x = year, y = realgrp, color = cluster)) +
    scale_y_continuous("Gross regional product (indexed to 2007)", labels=scales::percent) +
    scale_x_continuous("Year") +
    geom_line() + geom_text_lastonly(aes(label = realgrp))
  1. Similar question re formatting: could the formatting applied to the y axis apply to the label by default?

  2. in my test plot, which involves a legend, the legend gets modified by the addition of the label. See the image below.

  3. I forget where we left this off in our discussion last week - considering so much of the functionality is the same, is it worth incorporating adding a dot on the last point into the same function?

  4. is it within reason to automate the expand_scale() so it occurs within this function?

image

@matthewstern
Copy link
Contributor

updated the above comment a moment ago to correct display error in code blocks.

@nmpeterson nmpeterson mentioned this pull request Feb 10, 2020
6 tasks
@nmpeterson
Copy link
Contributor Author

I created a new issue (#24) with your suggestions. They don't relate to fonts and should be kept separate from this PR.

@matthewstern matthewstern merged commit 7b52aa0 into master Feb 10, 2020
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.

None yet

2 participants