jimhester / knitrBootstrap

A framework to create bootstrap styled HTML reports from knitr Rmarkdown.
Other
274 stars 61 forks source link

Update to bootstrap v3.3.7 & highlight.js v9.9.0 #193

Closed pat-s closed 7 years ago

pat-s commented 7 years ago

Changelog

Changes related to version update

R code outputs are now also highlighted - this was not the case before. One can use <pre><code class="nohighlight">...</code></pre> to disable highlighting using highlight.js. However, I do not know how to address the R code output class within your package/in general. The affected class is class="output r" which now has highlight.js inherited: class="output r hljs css". Before it was class="output r" within the div row.

Before:

screenshot 2017-02-13 23 27 09

Now:

screenshot 2017-02-13 23 27 39

humburg commented 7 years ago

Thanks for the pull request. There is an updated and in many ways improved version of knitrBootstrap in the rmarkdown_template branch (see #68 for related discussion). This already uses bootstrap 3 (albeit 3.2.0). You may want to take a look at that. It may be more productive to contribute any improvements to that branch, rather than master.

pat-s commented 7 years ago

@humburg thanks for the hint, did not know that.

Okay, I just saw that the source of the 'rmarkdown_branch' is different to the master branch in many points and my changes cannot simply be incorporated.

I changed the base for the PR and now there are quite some merge issues to be resolved. I guess I´ll open a new PR with includes my changes into the 'rmarkdown_template' branch.

I could open an issue at highlight.js to resolve the issue of the highlighted r code output (which I think is not useful). [I´ll check if this error also appears using your branch as the base]

I read #68. I really think this repo should go to CRAN even if Rmarkdown has improved greatly since then or most of the bootstrap themes are just too heavy. The feature of being able to use highlight.jsfor the code chunk highlighting is worth enough. Or is this also possible in basic Rmarkdown?

Also having the most updated version in the non-master branch for a long time now is confusing for guys like me who want to contribute. Is there any reason the master branch is not going to be updated?

EDIT: 'man' pages are missing in the 'rmardown_template' branch:

Downloading GitHub repo jimhester/knitrbootstrap@rmarkdown_template
from URL https://api.github.com/repos/jimhester/knitrbootstrap/zipball/rmarkdown_template
Installing knitrBootstrap
'/usr/local/Cellar/r/3.3.2/R.framework/Resources/bin/R' CMD INSTALL '/private/var/folders/5j/_1ts10x512sg_5q_3kzc_c1w0000gn/T/RtmpoohGAs/devtoolsda23c8e065e/jimhester-knitrBootstrap-7a9b8c7' 
No man pages found in package  ‘knitrBootstrap’
humburg commented 7 years ago

@pat-s thanks for your efforts. It would be good to get the highlighting issue resolved (if it persists).

The rmarkdown_template branch had a number of issues that prevented its release. I think that they were resolved, but I'll have to take a closer look to be sure. I agree that it should be released to CRAN, it's a shame to hide it in a place where most people miss it.

pat-s commented 7 years ago

@humburg I would need to raise an issue at highlight.js repo. Even after manually inspecting the div classes for both containers and trying to address the CSS classes, I did not find the difference between older highlight.js versions and the new one.

Also, because the base of those two branches has diverged so much, did you ever take into consideration putting your branch into a separate repo?

Of course this also depends on the plans of @jimhester regarding knitBootstrap in general.

humburg commented 7 years ago

@pat-s The rmarkdown_template branch was actually created by @jimhester some time before I started contributing. As far as I can tell there are no plans for further development. I've been adding features I needed/wanted for my own use of knitrBootstrap. I haven't done much work on this lately but if there are a few extra bits to polish off before a new release can be pushed out I'm happy to look at it again.

I'm a bit surprised by the highlight.js issues. I've been using highlight.js 9.9.0 in a related project without problems. It seems more likely that some CSS or javascript in knitrBootstrap is broken somewhere (although at first blush it isn't obvious to me).

pat-s commented 7 years ago

@humburg Your own project looks very promising, thanks for pointing me there! Actually, I liked the simple_document style most, so I guess it would be much more efficient to contribute to your new project rather than trying to fix knitrbootstrap. I just want to have the freedom to use whatever highlighting scheme I want together with some CSS mods.

Seeing that you are using highlight.js v9.9.0 with success, I would also assume that the error resides within knitrbootstrap.

So I´ll close this PR and will take a closer look at reportmd :smiley:

jimhester commented 7 years ago

@humburg I am happy to have you take over maintainership of this package, merge the PR and release to CRAN, you already have commit access to the repo. The things that are preventing me from merging and releasing a new version are detailed at https://github.com/jimhester/knitrBootstrap/pull/68#issuecomment-93811297. Mainly I don't want to release a new version that has fewer features than the previous CRAN version, as it is not trivial for most users to install older versions of packages.

I like the look of your related project, however if some of the code / ideas there are from knitrBootstrap some sort of acknowledgement of that fact would be appreciated.

As far as the contents of this PR I am not sure why you would want output highlighted like any source code? By definition output blocks are arbitrary text, not a particular language. In particular your example is highlighting the output as css, which is clearly incorrect.

pat-s commented 7 years ago

@jimhester Probably a misunderstanding. I did not want to highlight the output - this was an unwanted change introduced by simply updating highlight.js and bootstrap versions. I could not figure out the source of this change.

pat-s commented 7 years ago

@humburg The issue with output colouring also happens when using reportmd:

screenshot 2017-02-15 18 03 56

jimhester commented 7 years ago

@pat-s I don't know what you expect the output to be, but this is working as intended. The output block is currently highlighted as R code, but because knitr prefixes output with ## the entire output is highlighted as R comments.

pat-s commented 7 years ago

@jimhester You're right, this works as intended :neutral_face:. The problem is just the dark comment color of the theme here.

The issue with the syntax highlighting of code output I was referring to within the first post of this PR is not happening here.

humburg commented 7 years ago

@jimhester I'll try to find some time over the next few days to double check which of the issues regarding #68 are still open and see what I can do about that. At a quick glance, it seems to me that they have been mostly addressed.

You are right that you deserve some credit for knitrBootstrap's influence on reportMD. Some of the document format related code was certainly inspired by your work here. I'd be happy to add you as a contributor to the DESCRIPTION file if you like.