lebebr01 / highlighthtml Goto Github PK

Could you rework the example file bgtable.html so that it can load in the examples? I think this will be a stumbling block for beginners...

The goal of this package is to help users to style tables or text in HTML files. It requires the user to edit the source HTML by adding IDs to cells in a table or to specific text, and then using an R function to assign CSS styles to those IDS. While the package's aim seems useful , I find it somewhat difficult and unintuitive to use in its current form. I think spending some time on improving the documentation and explanations would help a great deal.

README comments (difficult to understand and contains many errors)

• First major comment is about clarity and readability of README. After reading the first paragraph, it's not at all clear to me where to use this package. Hightlight HTML tables... in shiny? in rmarkdown? Or literally in HTML files, but in that case it seems unrelated to R.

• There are also many typos in the README, missing words, and some awkward sentences. I saw the short paper.md file and that reads great! But the README, which is what most people will see when they find the package, is hard to understand unfortunately.

• In the first sentence in the Usage section for example: it's not clear to me how you get this table that looks like this -- am I supposed to look at the table's content or how it looks visually? Because of the description of the package, I kept thinking the look of that table is what mattered, but that was wrong.

• After reading the first sentence in "Usage" it's clear this is intended to be used on HTML pages produced from markdown. So that should be made clear in the intro.

• First sentence in usage: how do you get this table that looks like this? (do you mean visually or content-wise?)

• It would be nice to actually show what the input is and how the table looks initially, and then show with an image what the table looks like afterwards. Maybe if some of the wording changed from "table" to "dataframe" it could make it a bit more clear, because the word "table" is being used to describe both a rectangular dataset and the actual visual HTML table in an output. That ambiguity caused me a lot of confusion.

• I would add a note on how to install devtools since devtools is mentioned and people may not have it installed.

• In the example code in the README, the library(hightlightHTML) has a typo in the package's name

• When showing the example with the bgtable.html file, it would help if you showed us HOW you even created the input html file

• Is it update_css or updateCSS? Both versions are used in the README

• Why would I ever choose update_css=FALSE?

• Personal opinion: I would not overwrite the input file unless explicitly told so. It could be damaging to try out the function, forget you need to add an output , and lose your original file. Maybe default to <input_filename>-out.html to avoid overwriting the input file?

• The package's DESCRIPTION says it's also a tool to highlight text from an HTML document, but that's not mentioned at all in the entire README. The DESCRIPTION also mentions the ability to compile directly from Rmd but there's no documentation for that either

Vignette

-The vignette says that it is possible to highlight any text, not just HTML tables. So shouldn't the github tagline and the readme update to reflect that?

• table_id_inject() is briefly mentioned, which to me seems like an important function because this is an R package and this is the first R package and I still don't understand how to make a basic example in R

• It'd be helpful if you show the input file, because running file <- system.file(...) users won't know what's in there. How is the input supposed to look like?

Code

• Testing seems very light: test_structure.r has only one test, and all it looks for is expect_true(is.character(tmp)), it's not actually testing the structure. The function could be very wrong and give unexpected results but still pass the test.

• Most internal functions don't use any variable names, just x tmp tmpA tmpB etc so it'll be difficult to debug and maintain in the future

• rgb2hex(): documentation should say that the first param can be a named list but the second param is an unnamed list

• rgb2hex(): No error checking or error handling. Calling rgb2hex() results in an obscure unhelpful message Error in names(tmp) <- namescol : object 'tmp' not found

• I see a few checks for if(... & ...) and I think you probably want to use && instead of & (& is for vectorized comparisons, but I think these if statements want a single comparison)

• table_id_inject: the first param, x, says "A table object" -- do you mean a data.frame? That should be clarified what a table object is (since the word table is used a lot in this package and means other things). I would also use a better name than x

• conditions: I don't know what the syntax is or what conditions I can use. Based on the examples I know I can use '< x' or '> x', but can you use <=? Do you need to use one or two equals for equality? Can you use any other conditions (string matching? being BETWEEN two numbers?)

Overall, while the idea of the package sounds very useful, I'm having a hard time imagining the workflow to use it. If I have an HTML file with some tables in it, I don't see a simple way to modify the tables to include the IDs, and if it's not simple and effortless then I would be very unlikely to use the package.

Feel free to ignore any of the comments, these are obviously just my personal thoughts and it's possible some of them are wrong because of my own misunderstanding!

Erroneously did not test use cases when more than one match is returned.

This works:

library(dplyr)
library(highlightHTML)

chick_tab <- chickwts %>%
  group_by(feed) %>%
  summarise(avg_weight = mean(weight),
            sd_weight = sd(weight))

table_id_inject(chick_tab, id = c('#bggrey', '#bgblack'), 
                conditions = c('> 325', '< 165'),
                variable = list('avg_weight', 'avg_weight'))

This does not:

library(dplyr)
library(highlightHTML)

chick_tab <- chickwts %>%
  group_by(feed) %>%
  summarise(avg_weight = mean(weight),
            sd_weight = sd(weight))

table_id_inject(chick_tab, id = c('#bggrey', '#bgblack'), 
                conditions = c('> 270', '> 300'),
                variable = list('avg_weight', 'avg_weight'))

The issues is multiple conditional matches to the conditions argument.

Currently the following code will produce two CSS injections.

library(dplyr)
library(highlightHTML)

summ_table <- mtcars %>% 
  group_by(cyl) %>% 
  summarise(avg_mpg = mean(mpg), sd_mpg = sd(mpg)) %>%
  data.frame()
  
table_id_inject(summ_table, id = c('#bgred', '#bgblue'),
  conditions = c('< 2', '< 16'), 
  variable = list(c('sd_mpg'), c('avg_mpg', 'sd_mpg')))

Want to keep the last id.