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

too much space between each plot and its title #40

Closed
tdhock opened this issue Jun 12, 2020 · 13 comments
Closed

too much space between each plot and its title #40

tdhock opened this issue Jun 12, 2020 · 13 comments

Comments

@tdhock
Copy link
Collaborator

tdhock commented Jun 12, 2020

@lazycipher I noticed a new bug/regression. can you please add a test and work on a fix in #34 ?
In old animint2 which worked with R-3.6 we have very little space between each plot and its title:

screenshot-old-animint2-R-3 6

but in new animint2 we have

screenshot-too-much-space-animint2-R-4 0

the problem seems to be with the offset of the plot which is 111 here in the background y attribute

plot-offset-too-big-animint2

this is the first plot in test-renderer2-interactive-legends.R but this is happening with ALL animints I have seen with the new code.

@lazycipher
Copy link
Collaborator

Sure @tdhock, I'll work around this issue and report back as soon as possible.

@tdhock
Copy link
Collaborator Author

tdhock commented Jul 6, 2020

hi what do you need help with here? can you replicate the issue / see the problem in rendered data viz on your system?

@lazycipher
Copy link
Collaborator

Yes, my system also has the same issue.
Can you guide me on where should I look for in order to solve this issue?
I noticed that the title was using a css property, transform: translate(200, 17), which looked responsible for this issue.

@tdhock
Copy link
Collaborator Author

tdhock commented Jul 6, 2020

yes maybe that it is. try experimenting with changing those translate values interactively in the browser to see if that fixes the problem. then if it does, figure out where those values are being generated in the R/js code.

@lazycipher
Copy link
Collaborator

This line is causing issues! https://github.com/tdhock/animint2/blob/c2398111e8b0e73e73fac28a8d56d8cf3a912a4c/inst/htmljs/animint.js#L515
It renders correctly on R 3.6 but not on R 4.0. I guess it might be something related to some unit implementation.
I haven't found the R code responsible for passing these values but I'm sure I'll find this soon.

@tdhock
Copy link
Collaborator Author

tdhock commented Jul 8, 2020

thanks for your persistence/dilgence.
plotdim.margin comes from

    var margin = {
      left: 0,
      right: text_height_pixels * p_info.panel_margin_lines,
      top: text_height_pixels * p_info.panel_margin_lines,
      bottom: 0
    };

which comes from the following R code inside parsePlot:

  ## Now ggplot specifies panel.margin in 'pt' instead of 'lines'
  plot.info$panel_margin_lines <- pt.to.lines(theme.pars$panel.margin)

@lazycipher
Copy link
Collaborator

Thank you so much!
The value of theme.pars$panel.margin is 5.5pt in 3.6 while the value is 5.5points in 4.0.
The function pt.to.lines is not working as expected in 4.0.

@lazycipher
Copy link
Collaborator

It was the function pt.to.lines() which was having issues. Actually the issue was caused as we were trying to extract unit type by using attributes(pt_value)$unit. In the new grid, there's a new function unitType() to extract that.

several packages were extracting attributes from “unit” objects, e.g., the "mm" from unit(1, "mm"); there is a new grid::unitType() function that may help packages to avoid accessing ‘grid’ unit internals in the future.
ref: https://developer.r-project.org/Blog/public/2020/04/13/changes-to-grid-units/index.html

@tdhock
Copy link
Collaborator Author

tdhock commented Jul 9, 2020

did you add a test that fails before your fix? if not can you do that please? (always a good idea to write a test that fails when you find a bug, before you fix the bug)

@lazycipher
Copy link
Collaborator

Oh! I'm sorry! I forgot!
I'll add this asap.

@lazycipher
Copy link
Collaborator

lazycipher commented Jul 9, 2020

@tdhock, One thing I have to ask, this test will be a renderer one or what?
Calculating distance between title and plot is easy but with what we'll be comparing to make sure it's not big enough!
Or just add a test to make sure pt.to.lines functions working properly?

@tdhock
Copy link
Collaborator Author

tdhock commented Jul 9, 2020

you are right the renderer test is not super easy / straightforward.
anyway the bug was with the compiler (r code) so you should probably just test pt.to.lines, or test the panel_margin_lines attribute of the compiled viz list, see other test-compiler* files.

@lazycipher
Copy link
Collaborator

lazycipher commented Jul 10, 2020

Alright! Let me go ahead with the compiler test!

@tdhock tdhock closed this as completed Jul 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

No branches or pull requests

2 participants