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

Doubly check the manual flag #1957

Merged
merged 4 commits into from
Sep 3, 2019
Merged

Doubly check the manual flag #1957

merged 4 commits into from
Sep 3, 2019

Conversation

TimWolla
Copy link
Contributor

@TimWolla TimWolla commented Jul 1, 2019

Currently the manual is only checked before attaching the event
listener, making it hard to reliably set the flag without jumping
through hoops and special cases if Prism is bundled together with
other scripts into a single script file.
Change to check the flag once before attaching the event listener
and once again before actually highlighting to support setting it
after the Prism initialization has run.

see discussion starting at:
#1087 (comment)


Quoting from the discussion in the issue:

We can; we would break BC for people using the old document.removeEventListener('DOMContentLoaded', Prism.highlightAll) method of removing auto-highlighting, as now the callback we're attaching won't be just Prism.highlightAll.

and

Fine by me; if we do that, we should document that somewhere though. Is the CHANGELOG enough or should we include a "how to disable auto-highlighting" somewhere?

Should I make some other adjustments anywhere? /cc @mAAdhaTTah

@TimWolla TimWolla mentioned this pull request Jul 1, 2019
@TimWolla
Copy link
Contributor Author

TimWolla commented Jul 1, 2019

Misunderstood the requirement to not touch anything outside of components/ and did not run gulp. Now fixed and force pushed.

@TimWolla
Copy link
Contributor Author

This pull request is the only PR that is not labeled at all, so it looks like it might have slipped through the cracks. Can you take a look, please?

Copy link
Member

@RunDevelopment RunDevelopment left a comment

Choose a reason for hiding this comment

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

Sorry for the delay @TimWolla!

All in all, I'm ok with this aside from a few minor things (see comments).

Regarding the documentation:
I think we definetly need the doc (#1402) and should put this on your front page in the basic usage section (preferably above the CDN section).
Two or three sentences with one or two code example should be enough to sell the idea. The doc should mention Prism.manual and data-manual.

/cc @mAAdhaTTah

components/prism-core.js Outdated Show resolved Hide resolved
components/prism-core.js Outdated Show resolved Hide resolved
Copy link
Contributor Author

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

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

I've made the requested adjustments to the code.

components/prism-core.js Outdated Show resolved Hide resolved
components/prism-core.js Outdated Show resolved Hide resolved
@TimWolla
Copy link
Contributor Author

TimWolla commented Jul 24, 2019

I think we definetly need the doc and should put this on your front page in the basic usage section (preferably above the CDN section).

This can simply go into index.html? I notice that it already says:

f you want to prevent any elements from being automatically highlighted, you can use the attribute data-manual on the <script> element you used for prism and use the API. Example:

So only Prism.manual would need to be added, no?

@RunDevelopment
Copy link
Member

This can simply go into index.html?

Yes.

I didn't notice it already mentioned data-manual. What an attentive reader I am...
In that case, just Prism.manual and how data-manual affects Prism.manual is enough.

@TimWolla
Copy link
Contributor Author

In that case, just Prism.manual and how data-manual affects Prism.manual is enough.

Okay, I've added the docs as well. Let me know if there's something else I forgot!

index.html Outdated Show resolved Hide resolved
index.html Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
Copy link
Member

@RunDevelopment RunDevelopment left a comment

Choose a reason for hiding this comment

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

I would still like to have @mAAdhaTTah approve this.

@mAAdhaTTah
Copy link
Member

Just need to resolve the conflicts.

Currently the `manual` is only checked before attaching the event
listener, making it hard to reliably set the flag without jumping
through hoops and special cases if Prism is bundled together with
other scripts into a single script file.
Change to check the flag once before attaching the event listener
and once again before actually highlighting to support setting it
after the Prism initialization has run.

see discussion starting at:
#1087 (comment)
@TimWolla
Copy link
Contributor Author

TimWolla commented Sep 3, 2019

Just need to resolve the conflicts.

@mAAdhaTTah I've rebased my branch onto PrismJS:master and resolved the conflicts.

@RunDevelopment RunDevelopment merged commit d49f0f2 into PrismJS:master Sep 3, 2019
@RunDevelopment
Copy link
Member

Thank you for contributing @TimWolla!

@@ -533,21 +533,21 @@ if (script) {
_.filename = script.src;

if (!_.manual && !script.hasAttribute('data-manual')) {
_.highlightAutomaticallyCallback = function () {
function highlightAutomaticallyCallback() {

Choose a reason for hiding this comment

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

After this change it's not possible to minify script using uglify-js and "use strict" declaration:

  13971 |
  13972 |       if (!_.manual) {
> 13973 |               function highlightAutomaticallyCallback() {
        |       ^ In strict mode code, functions can only be declared at top level or immediately within another function.
  13974 |                       if (!_.manual) {
  13975 |                               _.highlightAll();
  13976 |                       }

Functions shouldn't be declared inside the if code block.

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

Successfully merging this pull request may close these issues.

4 participants