-
Notifications
You must be signed in to change notification settings - Fork 335
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
fix bug in prepend_class #868
Conversation
Can you merge/rebase? |
remerged. |
@@ -83,7 +83,7 @@ tweak_tables <- function(html) { | |||
} | |||
|
|||
prepend_class <- function(x, class = "table") { | |||
if (length(x) == 0) { |
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.
If this is the require fix, please write a unit test for prepend_class()
rather than testing the building of a complete website.
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.
I didn’t want to change @jayhesselberth test. I’ll look how he set it up.
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.
these are the tests that are used for prepend_class currently https://github.com/r-lib/pkgdown/pull/832/files#diff-a1049f633ef139157fcd18edf1f4fbac
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, I'd recommend adding a new test along those lines.
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.
Also this should have spaces around %in%
…es in the table definition
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.
Can you please remove all the files from the commit that are unrelated to the change? And please add a bullet to NEWS? It should briefly describe the change and end with (@yourname, #issuenumber)
.
@@ -83,7 +83,7 @@ tweak_tables <- function(html) { | |||
} | |||
|
|||
prepend_class <- function(x, class = "table") { | |||
if (length(x) == 0) { |
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.
Also this should have spaces around %in%
NEWS.md
Outdated
@@ -67,6 +67,9 @@ | |||
|
|||
* `build_home()`: a link to the source `inst/CITATION` was added to the authors page (#714). | |||
|
|||
* `prepend_class()`: Generalized to allow `tweak_tables()` to handle multiple |
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.
prepend_class()
is not exported so this is confusing to a reader.
Can you make it:
* Tables with multiple attributes are now properly handled (@yonicd #868).
bug related to issue #824.
prepend_class
has a logical check forlength(x)>0
, but it doesn't take into account if the attributes of the table had more than class in it, likestyle
.If that is the case then you are trying to access a name in the list of
x
that doesn't exist and it crashes.