-
Notifications
You must be signed in to change notification settings - Fork 26
theme_animint(rowspan, colspan, last_in_row) #153
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
base: master
Are you sure you want to change the base?
Conversation
Currently, animint renders plots sequentially without control over their positioning. Users need a way to specify row/column placement of plots. My approach is to add three new theme_animint() parameters:
Implementation: JavaScript Renderer: - Replace sequential rendering with a table-based grid
|
for gsoc you should be writing code + pushing + writing in issues or prs every day. |
…ed a new test for verifying rowspan functionality across multiple plots.
I tried implementing the rowspan and colspan layout feature it seems theme_animint(rowspan = 2) may not parsed properly by this code theme_attribute <- function(theme){
options_list <- list()
for(attributes in c("rowspan", "colspan", "last_in_row")) {
arc <- paste0("animint.", attributes)
options_list[[attributes]] <- if(arc %in% names(theme)){
theme[[arc]]
}
}
options_list
} so it’s not getting added to the JSON or rendered in the browser as . So i thought to referred to siddhesh’s pr, but there also the row, col, rowspan, and colspan attributes aren’t appearing in the html output. So how did you test this implementation @siddhesh195 can you please provide suggestion? and now instantly facing this issue tests_init("chromote")
Error in stop_servr(tmpPath = find_test_path()) :
could not find function "stop_servr"
Called from: tests_exit()
Browse[1]>
> tests_init()
Error in stop_servr(tmpPath = find_test_path()) :
could not find function "stop_servr"
Called from: tests_exit()
Browse[1]>
> tests_exit()
Error in stop_servr(tmpPath = find_test_path()) :
could not find function "stop_servr" @tdhock can you please suggest what should I do? |
@@ -0,0 +1,76 @@ | |||
test_that("check rowspan, all plots in single row", { | |||
plot_of_3_dots_data <- data.frame(x = c(1, 2, 3), y = c(1, 4, 9)) | |||
plot_of_2_dots_data <- data.frame(x = c(1, 2), y = c(1, 4)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please avoid numbers in variable names, and use informative variable names instead.
please read https://docs.google.com/document/d/1W6-HdQLgHayOFXaQtscO5J5yf05G7E6KeXyiBJFcT7A which has coding guidelines in R.
This is a violation of:
-5 for variable names and/or identifiers containing numbers/digits where names or another programming technique could be used instead. solution: use informative (self-documenting) variable/identifier names or another programming technique.
plot_of_1_dot_data <- data.frame(x = c(2), y = c(2)) | ||
plot_of_4_dots_data <- data.frame(x = c(1, 2, 3, 6), y = c(1, 4, 9, 13)) | ||
plot_of_5_dots_data <- data.frame(x = c(1, 2, 3, 6, 9), y = c(1, 4, 9, 13, 16)) | ||
plot_of_6_dots_data <- data.frame(x = c(1, 2, 3, 6, 9, 11), y = c(1, 4, 9, 13, 16, 18)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please try to simplify and reduce repetition.
if there needs to be a difference between these data sets, please make a comment to explain why.
otherwise please just use the same data set.
|
||
plot_of_1_dot <- ggplot(plot_of_1_dot_data, aes(x, y)) + | ||
geom_point(size = 2) + | ||
ggtitle("Plot of 1 Dot") + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-5 for repetitive code blocks / variable names that could be replaced by a loop or another less repetitive programming technique. solution: use for loop and a list with named elements, or a list of data tables, or another programming technique (repetition can and should always be avoided in programming).
|
||
plot_list <- list( | ||
plot1 = plot_of_1_dot, | ||
plot2 = plot_of_2_dots, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please avoid declaring variables above and then using them only once here.
bad:
variable <- something
plot_list <- list(plot1=variable)
good:
plot_list <- list(plot=something)
please undo addition of rds file |
https://github.com/animint/animint2/blob/458071391e567ae39fb1cdcc49a18a229fd24b62/tests/testthat/test-renderer3-fiveplots.R now this error cames tests_init("chromote")
Error in stop_servr(tmpPath = find_test_path()) :
could not find function "stop_servr"
Called from: tests_exit()
Browse[1]>
>
> tests_init()
Error in stop_servr(tmpPath = find_test_path()) :
could not find function "stop_servr"
Called from: tests_exit() so manually added those function > stop_servr <- animint2:::stop_servr
> start_servr <- animint2:::start_servr
> tests_init() its working |
hello @biplab-sutradhar , please take a look at this commit 6999ac7 and see if it gives you some starting point of how to see row,col and span in html. I had made that commit around a year ago during GSoC 2024 while working on the issue (#115 ) for which this PR exists, also please paste the link of any PR you are referring instead of mentioning like "siddhesh’s pr". thank you |
plot4 = plot_of_4_dots, | ||
plot5 = plot_of_5_dots, | ||
plot6 = plot_of_6_dots | ||
LargeLeftPanel = ggplot(base_data, aes(x, y)) + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is better thanks
@biplab-sutradhar i see that you have already added a commit (2bb97e8) for trying to put parameters like row, col, rowspan etc. for each plot in the html. sorry for missing that. your approach seems to be in the right direction. i need to see my PR (#139) again to know how I was able to put those parameters in html. also, since i was not following animint for some time, i need to ramp up on the new changes relevant to issue (#115), which this PR is trying to address the simplest way you can check is just manually analyze the generated html in your browser using inspect. if your approach is right they should be there. |
|
try using the Javascript debugger in your browser. click on the line of javascript code where you want to stop with a breakpoint, and inspect the variables there. |
@tdhock I have one question if I make new feature in animint.js file in \AppData\Local\R\win-library\4.4\animint2\htmljs\animint.js then I can test my changes directly in the browser. |
good question! if you edit animint.js in the rendered data viz, you need to copy that to animint2/inst/htmljs/ and then re-install the package. (R CMD INSTALL animint2) |
yes, very good! -5 for variable names and/or identifiers containing numbers/digits where names or another programming technique could be used instead. solution: use informative (self-documenting) variable/identifier names or another programming technique. CH31 |
@@ -145,6 +145,18 @@ getWidthAndHeight <- function(theme){ | |||
options_list | |||
} | |||
|
|||
theme_attribute <- function(theme) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems to be a copy of getWidthAndHeight in z_animintHelpers.R
getWidthAndHeight <- function(theme){
options_list <- list()
for(wh in c("width", "height")){
awh <- paste0("animint.", wh)
options_list[[wh]] <- if(awh %in% names(theme)){
theme[[awh]]
}else{
400
}
}
options_list
}
which is used in parsePlot() in z_animint.R:
options_list <- getWidthAndHeight(plot$theme)
please refactor your additions to avoid duplicated logic with this existing code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in getWidthAndHeight function extract c("width", "height"); in theme_attribute extract c("rowspan", "colspan", "last_in_row")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I create a single function to prevent logic duplication?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes please
inst/htmljs/animint.js
Outdated
p_info.plot_id = p_name; | ||
}else{ | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please click Files Changed tab, and undo any line changes which only involve white space, like this one
inst/htmljs/animint.js
Outdated
var tdRight = plot_tr.append("td").attr("class", p_name+"_legend"); | ||
if(viz_id === null){ | ||
var tdRight = plot_tr.append("td").attr("class", p_name + "_legend"); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please undo addition of empty lines in the middle of functions like this
inst/htmljs/animint.js
Outdated
@@ -243,23 +245,38 @@ var animint = function (to_select, json_file) { | |||
// Save this geom and load it! | |||
update_geom(g_name, null); | |||
}; | |||
|
|||
var current_tr = plot_td.append("tr"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use consistent indentation with surrounding code
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #153 +/- ##
==========================================
- Coverage 73.23% 63.97% -9.26%
==========================================
Files 163 163
Lines 8568 6016 -2552
Branches 488 0 -488
==========================================
- Hits 6275 3849 -2426
+ Misses 2293 2167 -126
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
xlab("X Axis") + ylab("Y Axis") + | ||
theme_animint(row = 2, col = 1) | ||
|
||
data_eight_dots <- data.frame(x = c(1, 2, 3, 6, 9, 11, 12, 13), y = c(1, 4, 9, 13, 16, 18, 19, 21)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please refactor these tests to avoid numbers in plot names.
why do you need so many plots, all with different data?
please explain, or (preferably) simplify.
geom_point() + | ||
ggtitle("left_plot") + | ||
theme_animint(rowspan = 2), | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please undo addition of empty lines in the middle of function calls in R code too
The World Bank visualization code is breaking and not understanding how layout changes are causing the issue. |
@tdhock can you help please |
closes #115
this proposal supersedes #139 which should be closed if this PR is sufficient.
The idea is to define three new plot attributes, to be specified inside
theme_animint
rowspan=2
means plot takes up two rows (default=1)colspan=2
means plot takes up two columns (default=1)last_in_row=TRUE
means this plot ends a row, HTML<tr>
element. (default=FALSE)the R code compiler needs to save these as attributes of the corresponding plot list.
For example
should make HTML like below

if any of these three attributes are specified in any of the ggplots, then the renderer JavaScript code puts the plot in the corresponding tr and td of the "outer table," according to the following rules:
<tr>
before the first plot, and after every plot withlast_in_row=TRUE
<td>
inside the current<tr>
There is no need for computing the total/max number of rows/columns.
@siddhesh195 please consider this PR and if you agree, close the other PR, and move any relevant code to this branch (compiler R code, tests, etc).