ccampbell / rainbow

Simple syntax highlighting library written in javascript
http://rainbowco.de
Apache License 2.0
3.3k stars 467 forks source link

Rainbow doesn't respect nodes without "data-language" #104

Closed nijikokun closed 8 years ago

nijikokun commented 11 years ago

It chooses all pre and code elements even if they are already highlighted and re-highlights or escapes the previously highlighted html.

I have a commit in line to fix this.

ccampbell commented 11 years ago

Rainbow should never re-highlight. It adds a class to each element when it is highlighted and if the element has that class it does not get processed again. Do you have an example?

nijikokun commented 11 years ago

Here you go:

https://www.mashape.com/nijikokun/parsify#getting-started

Pre-highlighted by parsify gfm, and re-highlighted by rainbow.

It doesn't respect pre/code that only have "data-language" attribute, I think I may be wording this wrong. It doesn't respect pre/code elements that DONT have data-language attribute.

My code change was lines 671 - 689:

        var pre_blocks = node.querySelectorAll('pre[data-language]'),
            code_blocks = node.querySelectorAll('code[data-language]'),
            i,
            final_blocks = [];

        // @see http://stackoverflow.com/questions/2735067/how-to-convert-a-dom-node-list-to-an-array-in-javascript
        // we are going to process all <code> blocks
        for (i = 0; i < code_blocks.length; ++i) {
            final_blocks.push(code_blocks[i]);
        }

        // loop through the pre blocks to see which ones we should add
        for (i = 0; i < pre_blocks.length; ++i) {

            // if the pre block has no code blocks then process it directly
            if (!pre_blocks[i].querySelectorAll('code[data-language]').length) {
                final_blocks.push(pre_blocks[i]);
            }
        }
ccampbell commented 11 years ago

Oh, unless I am misunderstanding, that is by design. If you don't include the language attribute then it doesn't touch the code. You can also use class="lang-whatever" or class="language-whatever" if you want.

nijikokun commented 11 years ago

I changed my to support data-language only, this way it doesn't conflict with other highlighters or static highlighting. Maybe a strict model so that way it doesn't conflict with others.

Plus, it's not stated on the site that it has this functionality so dropping support of it isn't much of a big deal.

ccampbell commented 11 years ago

Hmm, well querySelectorAll is not going to work on IE or other older browsers (not that rainbow works particularly well there either), but also this means that using a class will no longer work. I guess I am confused what the problem is.

If Rainbow finds a pre block without a language specified it should not do anything to it.

https://github.com/ccampbell/rainbow/blob/master/js/rainbow.js#L621

nijikokun commented 11 years ago

The box has a language specified but by class, not data-language attribute, I'm saying since your examples on the site only signify that data-language is used, that you shouldn't do classes as well, this was a quick hack.

ccampbell commented 11 years ago

Well I didn't originally support it, but it was added in #34, and I think makes sense to support.

nijikokun commented 11 years ago

I don't feel that is the right path to take, data attributes make more sense for checking which should be highlighted, and the class for which should be styled via CSS. Seperating logic from style.

This also conflicts with pre-styled code blocks.

nijikokun commented 11 years ago

I will bring this issue up with W3C as well.

ccampbell commented 8 years ago

Going to close this out since this is working as intended