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

Enhance definitions in TypeScript component #1522

Merged
merged 5 commits into from
Oct 18, 2018

Conversation

MichalLytek
Copy link
Contributor

No description provided.

@RunDevelopment
Copy link
Member

Thank you for this PR!

But you seem to have forgotten to add the minified files. Could you please commit them as well?

@MichalLytek
Copy link
Contributor Author

Sorry, I've used GitHub editor to edit the file in browser.
Will find contribution guidelines and fix this PR soon 😉

@MichalLytek MichalLytek changed the title Add readonly keyword to TypeScript component Enhance definitions in TypeScript component Aug 14, 2018
@RunDevelopment
Copy link
Member

If you want to complete the list of builtins and keywords you could also add the is and keyof keyword as well as the unknown and never builtin.

@MichalLytek
Copy link
Contributor Author

That makes sense, I've only checked my docusaurus documentation for missing stuffs.
I've pushed the suggested changes 😉

I miss the most the special keyword colouring feature (violet keywords on screenshot below) but I guess it would require changes in core to support this 😞

code_2018-08-14_21-14-54

@RunDevelopment
Copy link
Member

RunDevelopment commented Aug 14, 2018

I also think that the syntax coloring of VS Code is just gorgeous.

But you don't have to change the core for that. All you need is a little hook like this:

Prism.hooks.add('after-tokenize', function (env) {
	if (env.language !== 'typescript') {
		return;
	}

	for (var i = 0; i < env.tokens.length; i++) {
		var token = env.tokens[i];

		if (typeof token === 'string')
			continue;

		if (token.type === 'keyword' && /^(?:return|if|else|other special keywords)$/.test(token.content)) {
			var specialKeywordAlias = 'specialKeyword';

			if (!token.alias)
				token.alias = [specialKeywordAlias];
			else if (typeof token.alias === 'string')
				token.alias = [token.alias, specialKeywordAlias];
			else
				token.alias.push(specialKeywordAlias);
		}
	}
});

Add this hook before you do the highlighting and all keywords matching the regular expression will get an additional CSS class which can be used to change the color of certain keywords.

(This ought to work. I haven't thoroughly tested it though.)

@MichalLytek
Copy link
Contributor Author

@RunDevelopment Maybe I will make a separate PR with this feature in the future 😉

@RunDevelopment
Copy link
Member

@19majkel94 Maybe you should open an issue for that first.

This feature will not only change the feel of Prism (if we were to also change the themes) but it will also be quite hard to maintain with my quick and dirty solution.

A better way to achieve this would be to split the keyword lists into two and to mark special keywords with an alias. If we were to do this (and also were to change the themes), we should probably add this for a bunch of languages to keep the feel of Prism consistent.

I think?
Others might have better ideas (that don't require to change like 50 languages...).

@mAAdhaTTah
Copy link
Member

Can we get tests for the new keywords?

@MichalLytek
Copy link
Contributor Author

Can we get contribution guide with the info about how to write tests for the new keywords?

@mAAdhaTTah
Copy link
Member

Captured in #1533. You should be able to check out the other .feature files and see how the top part (where the code goes) translate to the bottom part (the list of tokens). Keywords should be fairly straightforward, but let me know if you have issues.

@MichalLytek
Copy link
Contributor Author

@mAAdhaTTah The tests has been updated 🎉

module
declare
constructor
enum
Copy link
Member

Choose a reason for hiding this comment

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

Why were these removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not removed, just sorted alphabetically. I've added the missing cases too.

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see. enum appears twice on the list, so I didn't see it added.

@mAAdhaTTah mAAdhaTTah merged commit 1169562 into PrismJS:master Oct 18, 2018
@mAAdhaTTah
Copy link
Member

Thank you for the contribution!

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