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

Custom Class: New class adder feature #2075

Merged
merged 9 commits into from
Nov 16, 2019

Conversation

RunDevelopment
Copy link
Member

@RunDevelopment RunDevelopment commented Sep 27, 2019

This adds a function to let the user define custom classes for specific tokens. Example:

Prism.plugins.customClass.add(function ({content, type, language}) {
	if (content === ':' && type === 'punctuation' && language === 'javascript') {
		return 'colon';
	}
});

(You can also define multiple such functions.)

The whole thing is implemented via the wrap hook, so content is the innerHTML of the token. I choose this over before-tokenize because for users innerHTML is easier to understand than the Prism token stream.

Further considerations

This feature is kind of a superset of Highlight keywords.
So maybe we could combine the two edit: merge HK into CC deprecate HK?


@MattMcFarland I hope this helps.

@MattMcFarland
Copy link

Looks promising!

@mAAdhaTTah
Copy link
Member

My apologies–could you remind me what sparked this PR?

@RunDevelopment
Copy link
Member Author

The idea was some people want to selectively highlight certain tokens differently from what we do.

In the above example (which was pre #2073), the colon : should be highlighted like an operator in JS. With this PR, people can do it without changing the language definition.
Another example is how #1743 includes keyword groupings and special names for certain operators for more granular highlighting.

The main purpose is to give the designers of themes more freedom, so they aren't limited to the way we choose to tokenize. Of course, you can also achieve this by modifying the grammar but that's a lot harder.

Copy link
Member

@mAAdhaTTah mAAdhaTTah left a comment

Choose a reason for hiding this comment

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

The API change is def necessary cuz content is probs the least useful aspect to add additional classes. Alternatively, we could pass it in as an object with the three properties, which would eliminate the question of what order to make them.

@RunDevelopment
Copy link
Member Author

Also, regarding the API:
I don't like that prefix and map set the current prefix or map but add adds another mapper. So either we change the name to indicate this difference or we change the behaviour.
Thoughts?

@mAAdhaTTah
Copy link
Member

Yeah, it kinda feels like map is a subset of what add does. Can we change map to use add such that it always adds a new mapper?

@RunDevelopment
Copy link
Member Author

No, because:

  1. rn, add can't modify existing classes. It doesn't even see them.
  2. there is no method to remove the functions passed to add, so we can't overwrite the mapper.

@mAAdhaTTah
Copy link
Member

Ok... maybe it makes more sense for add to just take a single function then, and overwrite the previous one? Since we didn't see a need for making the first 2 take multiple functions, and we haven't gotten complaints about it, it makes sense to keep them aligned by just having the single add function.

@RunDevelopment
Copy link
Member Author

Sounds good, but this won't make add a superset of map. Is that still ok?

@mAAdhaTTah
Copy link
Member

Yeah, I don't think that's a necessary goal at this point. At least this makes the behavior consistent.

@RunDevelopment
Copy link
Member Author

RunDevelopment commented Nov 15, 2019

Sorry for the late responses. I've been a little busy lately.

The only things stopping v1.18 are #1998 and #2074. TBH, #2074 modifies Core and isn't as important as #1998, so I'm fine with leaving it out of this release if it'll mean that we can release faster.

I'll do the changelog in the meantime.

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.

3 participants