Skip to content
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

Code highlighting feature #304

Closed
wants to merge 13 commits into from
Closed

Conversation

lekzd
Copy link

@lekzd lekzd commented Feb 5, 2018

Topics to discuss:

  1. how to make gulp task with highlighting separately (problems with text replacing in index.html and with highlight already highlighted file)
  2. switching off highlighting of <pre><code></code></pre> snipeets without any class? (class needed to highlight as specific language, default is HTML)

fixed lint warning for forgotten semicolon in gulpfile
add highlightJs gulp plugin
add copying of styles for highlighting
add highlighting to gulp prepare task
one performance tweak for gulp task
.gitignore Outdated
@@ -1,3 +1,4 @@
.idea
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It’s not related to project’s code, otherwise it might fill with VS Code, Sublime, Atom, Notepad++, etc. configs. There’s global .gitignore for that.

@pepelsbey
Copy link
Member

pepelsbey commented Feb 6, 2018

Suggestion: do not apply HTML highlight to every <pre> by default, but look for explicit class names with a proper language name and do the job only if it’s found. The same idea applies to already highlighted case: if there’s a language name and there are no classes inside of this <pre> then stop the script. Is there a way to detect of code is already highlighted?

@lekzd
Copy link
Author

lekzd commented Feb 6, 2018

@pepelsbey at first, I have selector for code for highlighting '.slide pre:has(code)', not even 'pre' tag would be highlighted.
at second, I can set any attribute of already highlighted snippet, maybe it can help.

@lekzd
Copy link
Author

lekzd commented Feb 13, 2018

@pepelsbey looks like all points regarding our talk are implemented.

@lekzd
Copy link
Author

lekzd commented Mar 14, 2018

@pepelsbey any updates?

@pepelsbey
Copy link
Member

Couldn’t find time to properly test it. Will try to have a look this week. Sorry.

@pepelsbey
Copy link
Member

Right, I’m back. We have a few problems here, but nothing too scary :)

  1. Fork is a bit outdated, since there’s been 2.5.0 release. I tried to merge it but got stuck with package-lock.json. Will you have some time to update your PR with the latest master? I know it’s totally my fault with outdated fork situation. But we can still make it work!

  2. I’d prefer simpler naming: rather highlight than highlightCodeSnippets both for script and variables. The same applies to highlightStyles, copyStyles, highlightFiles. There’s no right or wrong here, but you can clearly see the difference merge(shower, core, themes, highlightStyles). Like there were two different people writing this code. Oh wait :)

@lekzd
Copy link
Author

lekzd commented Jul 3, 2018

@pepelsbey looks like all was done

@@ -0,0 +1,100 @@

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not useful empty line

package.json Outdated
"gulp-replace": "^1.0.0",
"gulp-zip": "^4.0.0",
"gulp-zip": "^4.1.0",
"gulp-util": "^3.0.8",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use of the deprecated package https://medium.com/gulpjs/gulp-util-ca3b1f9f9ac5

whole gulp community is trying to remove it from projects :)

package.json Outdated
"merge-stream": "^1.0.0",
"path-exists-cli": "^1.0.0"
"path-exists-cli": "^1.0.0",
"run-sequence": "^2.2.1"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this a new package?

I do not see its use in PR

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It’s not used anymore since moving to Gulp 4.

gulpfile.js Outdated
`$1${STYLES_DEST_PATH}$3`, { skipBinary: true }
))
.pipe(highlightCodeSnippets())
.pipe(gulp.dest(''));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error: Invalid dest() folder argument. Please specify a non-empty string or a function.

It gives an error, I guess it should be '.'

gulpfile.js Outdated
/(<link rel="stylesheet" href=")(node_modules\/highlight\.js\/styles)\/(.*\.css">)/g,
`$1${STYLES_DEST_PATH}$3`, { skipBinary: true }
))
.pipe(highlightCodeSnippets())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It’s highlightCode() now.

package.json Outdated
"merge-stream": "^1.0.0",
"path-exists-cli": "^1.0.0"
"path-exists-cli": "^1.0.0",
"run-sequence": "^2.2.1"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It’s not used anymore since moving to Gulp 4.

@pepelsbey
Copy link
Member

pepelsbey commented Jul 3, 2018

You suggest in docs to run gulp highlight, but if prepared folder doesn’t exist yet you’ll just get highlight styles copied, but no real work will be done. I guess it should be a sequence: always prepare then highlight.

Vadim Makeev and others added 5 commits July 3, 2018 12:47
@lekzd
Copy link
Author

lekzd commented Jul 23, 2018

@pepelsbey btw I removed info about how to run code highlight separately from docs because it included to preparate task by default.

@pepelsbey
Copy link
Member

@nikolay-govorov, it seems code highlighting is another feature worth moving to CLI :)

@pepelsbey
Copy link
Member

@nikolay-govorov what do think about such feature for shower-cli?

@nikolay-govorov
Copy link

I think it would be cool to have this in the CLI, but I'm not sure I know how to integrate it. Is it going to be something like the --code-highlighting flag for prepare?

@pepelsbey
Copy link
Member

I’d just have it as shower highlight task.

@nikolay-govorov
Copy link

nikolay-govorov commented Sep 3, 2018

But what will she do? Now archive and publish are building the presentation, not looking in the prepared folder as it was in the old build. How to work the command highlight?

@pepelsbey
Copy link
Member

Hmm, then you’re right. It makes sense to use it as a flag, rather

shower prepare --highlight
shower archive --highlight

@pepelsbey
Copy link
Member

We decided that such a feature does not align with the project’s vision. Presentation’s HTML is a source file that should be easy to write and understand. A generator that modifies the source file would hurt source code readability. We’d rather go manual (mark) or dynamically generated (highlight.js) code highlight path.

I’m sorry it took the whole year to get back to this PR and decide.

@pepelsbey pepelsbey closed this Feb 12, 2019
@lekzd

This comment has been minimized.

@pepelsbey

This comment has been minimized.

@lekzd

This comment has been minimized.

@shvaikalesh
Copy link
Contributor

As @RubenVerborgh articulated in this comment, I don't see any reason for having syntax hightlight as either plugin or gulp task. Shower's code blocks have notion of lines (multiple <code> elements inside <pre>) you can mark. That seems like the only thing that requires special care, but it is not Shower-specific. We should rather extract this feature as custom element with hightlight.js built-in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants