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

Added general de/activation mechanism for plugins #2434

Merged
merged 1 commit into from
Jun 26, 2020

Conversation

RunDevelopment
Copy link
Member

The whole thing is implemented as a small isActive function prism-core.

This adds about 150bytes to Core and about 50bytes to Keep Markup but saves about 130bytes in LN and 40bytes in NW. So we gain a few bytes overall but we also gain functionality.


This resolves #2433.

@LeaVerou
Copy link
Member

LGTM. I may have done the implementation of isActive differently, e.g. in one of the following ways:

  1. Use element.closest(`.${className}, .no-${className}`) and then examine what class the result has
  2. Use element.closest(`.${className}`) and element.closest(`.no-${className}`) and then if both exist use element1.contains(element2)

But that's just optimization :)

@RunDevelopment
Copy link
Member Author

I never heard of closest but with it (and some newer JS syntax) you can do the whole function in a one-liner:

function isActive(element, className, defaultActivation) {
    return element?.closest(`${className}, no-${className}`)?.classList?.contains(className) ?? defaultActivation;
}

Unfortunately, my favorite browser doesn't support it.

@RunDevelopment RunDevelopment merged commit a36e96a into PrismJS:master Jun 26, 2020
@RunDevelopment RunDevelopment deleted the issue2433 branch June 26, 2020 22:01
@LeaVerou
Copy link
Member

I would avoid unsupported syntax, though I'm more lenient with unsupported functions, since they can always be polyfilled. And we're discussing dropping IE11 anyway, right?

Also if I'm reading the one liner above correctly, it looks like it would fail (would return true instead of false) if an element had no-classname and defaultActivation was true.

@RunDevelopment
Copy link
Member Author

And we're discussing dropping IE11 anyway, right?

Yes, but we haven't yet.

Hmmm. Maybe it's a good idea to have a tracking issue of all things that should be done after we drop IE11?

Also if I'm reading the one liner above correctly, it looks like it would fail (would return true instead of false) if an element had no-classname and defaultActivation was true.

Nope, (false ?? true) === false. defaultActivation will only come into play if and any of the chained values is nullish.

@LeaVerou
Copy link
Member

Hmmm. Maybe it's a good idea to have a tracking issue of all things that should be done after we drop IE11?

Absolutely.

Nope,

Oh right.

quentinvernot pushed a commit to TankerHQ/prismjs that referenced this pull request Sep 11, 2020
Keep Markup, Line numbers, and Normalize whitespace now share the same per-element class-based de/activation logic.
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.

Class to escape keep markup on certain code elements
2 participants