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

add plugin custom class name #950

Merged
merged 6 commits into from
Sep 30, 2016
Merged

add plugin custom class name #950

merged 6 commits into from
Sep 30, 2016

Conversation

thien-do
Copy link
Contributor

@thien-do thien-do commented May 6, 2016

This plugin allows to replace Prism default class names (like .comment, .string, .property, etc) with your defined ones (like .editor__comment (BEM style) or ._7sh3a (CSS Modules hashed class)).

For more information, please see the plugin page: http://quoinex.github.io/prism/plugins/custom-class-name/

Issue: #947

@thien-do
Copy link
Contributor Author

thien-do commented May 6, 2016

@LeaVerou @zeitgeist87 I have some time to write the plugin as discussed previously with you two. Please take a look at it.

I also have one problem here: I create several separate files in my plugins folder to demo how this plugin works, but it seems that make uglify failed to run. Do you have any advice to fix this pull request?

@zeitgeist87 zeitgeist87 mentioned this pull request Jun 3, 2016
@zeitgeist87
Copy link
Collaborator

Sorry for the late response. I was quite busy last month. Looking over your source code I had the following idea. Your plugin could also offer the option of a prefix to all classes. Something like a namespace:

Prism.plugins.customClassName.prefix("mynamespace-");

That way the user would not have to provide a map for every possible class. You can of course still support the map. What do you think?

@thien-do
Copy link
Contributor Author

thien-do commented Jun 3, 2016

Oh right, I should have that. Actually that is my first solution when trying to isolate Prism's class (before I meet CSS Modules)

@zeitgeist87
Copy link
Collaborator

Your example JavaScript files like example-2.js cause the minifier to crash. You cannot have any extra *.js files in that directory.

Also your examples are very small. It is probably best if you include them inline in index.html.

if (
typeof self !== 'undefined' && !self.Prism ||
typeof global !== 'undefined' && !global.Prism
) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you have this wrong. What if both self and global are undefined? How about this:

(typeof self === 'undefined' || !self.Prism) && (typeof global === 'undefined' || !global.Prism)

@zeitgeist87
Copy link
Collaborator

Your plugin currently only maps a single token type to a single class, which is a bit limiting. Prism supports aliases, so env.classes could look like this:

env.classes = ['token', 'equation', 'string', 'important'];

Wouldn't it be better to map classes to classes?

env.classes = env.classes.map(function(c){
    return classMap[c] || c;
});

@LeaVerou
Copy link
Member

LeaVerou commented Jun 7, 2016

Sorry for the late response. I was quite busy last month. Looking over your source code I had the following idea. Your plugin could also offer the option of a prefix to all classes. Something like a namespace:

Prism.plugins.customClassName.prefix("mynamespace-");

That way the user would not have to provide a map for every possible class. You can of course still support the map. What do you think?

Accepting a function as a parameter that takes a class and returns the namespaced class would also be good for maximum customization (though I'm a bit worried it might be going towards feature creep :P)

@LeaVerou
Copy link
Member

LeaVerou commented Jun 7, 2016

Also, customClass would be a better I think, customClassName is a bit too verbose.

@thien-do
Copy link
Contributor Author

thien-do commented Aug 14, 2016

Hi @zeitgeist87 and @LeaVerou , I'm very sorry for the late response. I finally have time to enhance the pull request as your suggestions. Please take a look and tell me anything that I need to fix.

Summary changes:

@thien-do
Copy link
Contributor Author

Hi @Golmote , would you mind taking a look at this? It seems @zeitgeist87 and @LeaVerou don't have time for this :(

@Golmote
Copy link
Contributor

Golmote commented Sep 27, 2016

@dvkndn I only had time to look through the code right now. It looks quite good to me. I'll find a moment this week to actually test your plugin.

I think I spotted some grammar errors in your plugin HTML page. I'm not an native English speaker myself, though, so I'm not really the best candidate to help fixing this.

Copy link
Contributor

@Golmote Golmote left a comment

Choose a reason for hiding this comment

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

This is the best I can do to help address this grammar issue ;)

<section>
<h1>Motivation</h1>

<p>Prism default classes are sensible but fixed and too generic. This plugin provide some ways to customize those classes to suite your needs. Example usages:</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't it be "suit" instead of "suite"?


<ul>
<li>
You want to add namespace for all of them (like <code>.prism--comment</code>) to avoid conflict with your existed classes.
Copy link
Contributor

@Golmote Golmote Sep 27, 2016

Choose a reason for hiding this comment

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

I should be "existing" instead of "existed".

comment: 'comment_93jsa'
})</code></pre>

<p>Object's keys are the tokens you want to replace (eg: <code>comment</code>), with its value is the classes you want to use (eg: <code>my-comment</code>). Tokens which are not specified will stay the same.</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

"with its value is the" should probably be replaced with something like... "with their values being the", I guess...


<h2>CSS Modules Usage:</h2>

<p>The initial purpose of this plugin is to use with CSS Modules. It works perfectly with the class map object returned by CSS Modules. For example:</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

"is to use" should probably be replaced by "is to be used".

@thien-do
Copy link
Contributor Author

Thank you @Golmote :D I fixed those grammar errors as you spotted. Sorry for my bad English :(

@Golmote
Copy link
Contributor

Golmote commented Sep 30, 2016

Ok, it looks good to me.

One last thing... why is components/prism-markup.min.js listed in the changed file? Can you revert the changes on this file?

@thien-do
Copy link
Contributor Author

Sure I reverted the file. That is an accident, sorry

@Golmote
Copy link
Contributor

Golmote commented Sep 30, 2016

Great! Merging, then!

@Golmote Golmote merged commit a0bd686 into PrismJS:gh-pages Sep 30, 2016
@jGleitz
Copy link

jGleitz commented Oct 16, 2016

Why was this meged into gh-pages? Will it be merged into master and published to npm any time soon?

@Golmote
Copy link
Contributor

Golmote commented Oct 16, 2016

Because everything is merged to gh-pages first. I'll try to make a release on master this month if nobody does it before.

@jGleitz
Copy link

jGleitz commented Oct 16, 2016

I see. Thanks!

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.

5 participants