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

Scoping highlighting tags to the mocha element #1325

Merged
merged 1 commit into from
Aug 30, 2014

Conversation

kentcdodds
Copy link

I'm not sure whether I'm missing an edge case or not, but this solves my issue and it makes sense to me that this is what we would want it to be anyway. Everything should be scoped to the #mocha element anyway. My use case is I have a collection of components and I have tests for them. I have a site that shows the component, an example, and the test. The example and test is interactive and runs on the page. Without this change, mocha will highlight all my other code tags on the page which messes things up. Here's an example of this change working on my page: http://kent.doddsfamily.us/kcd-angular/#/kcd-dynamic-attr#spec (see the spec toward the bottom)

I'm not sure whether I'm missing an edge case or not, but this solves my issue and it makes sense to me that this is what we would want it to be anyway. Everything should be scoped to the `#mocha` element anyway. My use case is I have a collection of components and I have tests for them. I have a site that shows the component, an example, and the test. The example and test is interactive and runs on the page. Without this change, mocha will highlight all my other code tags on the page which messes things up. Here's an example of this change working on my page: http://kent.doddsfamily.us/kcd-angular/#/kcd-dynamic-attr#spec (see the spec toward the bottom)
@kentcdodds
Copy link
Author

Here's a little demo of what this allows me to do...

demo

Notice that I'm running the tests inside a div in the rest of my page. This commit lets me scope my code tags to that section.

@boneskull
Copy link
Contributor

I'm actually kind of surprised this code exists at all. If the user wants syntax highlighting, it's trivial to apply highlight.js or whathaveyou. And assuredly a 3p module will do a better job than this wonky highlight function...

I'd just as soon remove this functionality and let the user do what they want, instead of standing in their way (or at least allow the user to turn it off)...

@kentcdodds Would you be interested in changing your PR to make the automatic highlighting optional? Would that solve your problem?

@jbnicolai
Copy link

@boneskull @kentcdodds I think both would make sense. Scoping it to mocha and making it optional.

To be honest, I feel that most of the functionality offered in utils could be deprecated and optionally retrieved from other specialized npm packages. Why are we doing the ascii colouring ourself, for instance, when Chalk would do a far better job.

@kentcdodds
Copy link
Author

I thought this code was for highlighting the test. So I don't see how it could be removed or made opt-in without a breaking change version bump. Maybe I'm wrong. But I don't know this codebase well...

@boneskull
Copy link
Contributor

OK, let's accept this PR.

Created #1329 for making highlighting optional. Created #1330 for auditing utils.js.

boneskull pushed a commit that referenced this pull request Aug 30, 2014
Scoping highlighting tags to the mocha element
@boneskull boneskull merged commit 070a2fb into mochajs:master Aug 30, 2014
@jbnicolai
Copy link

👍 ⭐

@boneskull
Copy link
Contributor

Uh oh, Travis failed. I'd like to find someone who prefers the dot reporter to tell me why.

@boneskull
Copy link
Contributor

Transient failure, restarted the build and it's ok.

@jbnicolai
Copy link

A nice, explains why I couldn't reproduce it locally.

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.

4 participants