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

do not special case 1 group #41

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

do not special case 1 group #41

wants to merge 1 commit into from

Conversation

tdhock
Copy link
Owner

@tdhock tdhock commented Jun 24, 2021

hi @Anirban166 this PR contains a fix for the following example:

df <- data.frame(i=1:10, label="more
than
one
line
really
a
lot")
library(ggplot2)
gg <- ggplot(df, aes(i, i, color=label))+
  geom_line()
directlabels::direct.label(gg, "right.polygons")

When you run this code on master, the label text will be unreadable (going off the top of the plotting region).
Can you please add a test case for this sometime before the end of the summer?
The test case should basically say, the top of the polygon / text grobs should be at or below the top of the plotting region.
Does that make sense?

@tdhock
Copy link
Owner Author

tdhock commented Jun 24, 2021

before this PR (current master) makes the figure below (note the first/top line "more" is NOT readable)
directlabels-bug

after this PR makes the figure below (note the first/top line "more" is readable)
directlabels-bug-fixed

@Anirban166
Copy link
Collaborator

The test case should basically say, the top of the polygon / text grobs should be at or below the top of the plotting region.
Does that make sense?

Yep! Thanks for the example plot as well. (it's great that you discovered this, definitely looks out of place for a single label with large text)

Can you please add a test case for this sometime before the end of the summer?

For sure, I'll add it when I'm making the test cases 👍

@Anirban166
Copy link
Collaborator

@tdhock For e.g. this PR is passing the check because the commits are made by you (please merge both!)

@tdhock
Copy link
Owner Author

tdhock commented Jul 6, 2021

before merging this PR can you please push the test cases I described above?
I added you as a collaborator on tdhock/directlabels so you should be able to push to this branch now.
In the future please create new branches / PRs under tdhock/directlabels instead of your fork.

@Anirban166
Copy link
Collaborator

before merging this PR can you please push the test cases I described above?
I added you as a collaborator on tdhock/directlabels so you should be able to push to this branch now.
In the future please create new branches / PRs under tdhock/directlabels instead of your fork.

okay sure! Thanks for adding me as a collaborator, yep I'll drop the fork now!

@Anirban166
Copy link
Collaborator

Hi @tdhock
I was wondering, can we obtain the position (x and y values) of a drawn grid grob or more specifically a child grob which has been forced? For instance, consider our case here:

directlabels-bug

I would like to get the y-coordinate of the topmost side of the drawn polygon, which I'd like to compare with the plot's topmost point or maximum y-value (which should be another grob, probably a polyline) - the former should exceed the later.

I'm aware that currently it is not possible to work on the polygon grob and the text contained within (until #44 is done with) as because our dlgrob cannot be forced:
image
(^ dlgrob[GRID.dlgrob.1.right.polygons] is one consolidated grob with the current approach and not a gTree with children like we want it to be)

But still I am curious - like in general, we can edit the original properties of a drawn grob using grid.edit(), so is there some way we can extract the positions (x, y) of a drawn grob as well? (for something as simple as a line to start with?)

Also asking this because it is related to the way I thought to implement this test case - I would go with the code you supplied and use an expect_gt() with respect to the y-coordinate of the top side of the polygon and the top line of the plot panel respectively.

I drafted this, but I don't think this is what you had in mind:

library(testthat)
context("Single label group with large (multiline) text")
test_that("Top of the drawn polygon/text grobs (dlgrob) should be below the plotting region.", { 
df <- data.frame(i=1:10, label="more
than
one
line
really
a
lot")
library(ggplot2)
gg <- ggplot(df, aes(i, i, color=label)) + geom_line()
directlabels::direct.label(gg, "right.polygons")
# Single group: (not considered for collisions in qp.labels previously)
expect_equal(length(unique(df$label)), 1)
# Multiline:
expect_gt(nchar(gsub("[^\n]+", "", df$label[1])), 1)
})

I wonder what I should test for in this case?

@Anirban166
Copy link
Collaborator

@tdhock re-pinging here in case the above comment didn't pop up in your mail

@tdhock
Copy link
Owner Author

tdhock commented Aug 8, 2021

yes there should be some way to query the x,y position of a drawn grid grob, I don't remember how right now, please ask @pmur002 who is author of grid package.

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.

2 participants