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

Pretty print #288

Merged
merged 4 commits into from Jan 23, 2014
Merged

Pretty print #288

merged 4 commits into from Jan 23, 2014

Conversation

KevinCathcart
Copy link
Contributor

This pull request simply adds a noHighlightCSS config option to leave out the code highlighting style sheet (useful if using a custom style-sheet to avoid needing higher specificity to override the defaults).

I also split out the google-code-prettify code from the highlight.js file, and update it to the latest version.
The latest version now registers as an AMD module (in the same way as jquery), so utilize that fact rather than relying on the globals.

Unfortunately, because it self-registers like jquery, it must be referenced with the name google-code-prettify, which dictates the file name and location unless the require.js paths feature is used.

I'd also like to add the lang-css.js auxillary file to allow for highlighting CSS fragments. However, I'm not entirely sure where to put that. Should it go in js/core/, in js/, or somewhere else?

This is useful if a custom stylesheet already has the highlighting definitions integrated.
The latest versions are now AMD compatible, as long as the module name
`google-code-prettify` is used. Updated Highlight.js to use the
exported object.
@marcoscaceres
Copy link
Member

Please make sure you also prepare documentation for this new feature:

https://github.com/darobin/respec-docs

["text!core/css/highlight.css"],
function (css) {
["text!core/css/highlight.css", "google-code-prettify"],
function (css, PR) {
Copy link
Member

Choose a reason for hiding this comment

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

what is PR in this context? Might be better to use a name that is more clear for this argument?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is just the normal global name exported by recent versions of google-code-prettify. Following the example of jquery imports using $, i stuck with the name of the global. This makes it consistent with the language extentions like the lang-css.js that I mentioned.

I agree that it is hardly the most descriptive name, but then again neither is $ or _.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the clarification.

I agree that it is hardly the most descriptive name, but then again neither is $ or _.

Agree, but that's not an excuse to make our code illegible :)

Copy link
Member

Choose a reason for hiding this comment

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

If you don't mind @marcoscaceres, I'll be the judge of that. PR is a perfectly fine name to use here for all the reasons @KevinCathcart mentioned.

Copy link
Member

Choose a reason for hiding this comment

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

Of course I don't mind @darobin. I was just asking what it was and where it came from - it was not immediately clear to me, which is why I asked for the clarification (which also made sense). My comments about $ and _ being bad was unrelated to the clarification - sorry if that was not clear.

darobin added a commit that referenced this pull request Jan 23, 2014
@darobin darobin merged commit ce08812 into w3c:develop Jan 23, 2014
@darobin
Copy link
Member

darobin commented Jan 23, 2014

Thanks @KevinCathcart.

Re lang-css.js, since it isn't a ReSpec module I reckon it's best to put it in /js/.

shikhar-scs pushed a commit to shikhar-scs/respec that referenced this pull request Feb 19, 2018
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

3 participants