Parse Settings plugin #851

zeitgeist87 · 2016-01-07T09:47:30Z

This plugin is not intended to be used on its own. It is a support
plugin for other plugins and only included as a dependency.

All ancestors of the current element are automatically scanned
for data--attributes and classes. These attributes and classes
are then parsed into key-value pairs and inserted into the
env.settings object. An element can inherit the data--attributes
from its ancestors.

Other plugins can use the collected settings from the Prism
environment (env.settings).

This plugin is not intended to be used on its own. It is a support plugin for other plugins and only included as a dependency. All ancestors of the current element are automatically scanned for data-*-attributes and classes. These attributes and classes are then parsed into key-value pairs and inserted into the env.settings object. An element can inherit the data-*-attributes from its ancestors. Other plugins can use the collected settings from the Prism environment (env.settings).

zeitgeist87 · 2016-04-30T14:32:43Z

@LeaVerou @Golmote ping 😄

Golmote · 2016-04-30T14:59:25Z

Yay this looks like a great idea!

LeaVerou · 2016-05-01T01:40:21Z

plugins/parse-settings/index.html

+
+	<p>To avoid accidental matches the element containing the attributes has to be marked for
+	use by Prism. The mark can either be the boolean attribute <code>data-prism</code> or
+	a Prism language class (<code>&lt;pre class="language-javascript"></code>).</p>


Why do these elements need to be specifically marked? Can't we just check whether they are processed by Prism? No other plugin requires elements to be marked, unless it's to provide setting values. Perhaps I'm misunderstanding this, dunno.

Maybe my description is a bit unclear. The pre and code tags do not have to be marked:

<pre data-option1="something"><code data-option2="something"> // Code </code></pre>

Any tag with a prism language class does not have to be marked:

<div class="language-javascript no-setting1 setting2" data-left-trim="false">  <pre><code> // code block 1 </code></pre> <pre><code> // code block 2 </code></pre> </div>

Only if you want settings on some unrelated tag you have to mark it:

<div class="language-javascript">  <div data-prism data-left-trim="false"> <pre><code> // code block 1</code></pre> </div> <pre><code> // code block 2 </code></pre> </div>

LeaVerou · 2016-05-01T01:50:03Z

Thanks for implementing this, sorry it took so long to give feedback!

I think a better approach is to get the plugins to declare which settings they need and their values (e.g. non-boolean values don't need to even check class names) via an object literal, then the plugin would provide a method to get their values for a given element. This would avoid having to specially mark elements that contain data to avoid false matches, or the awkwardness and perf overhead of iterating over attributes. Having the plugins provide a settings spec would also allow for cool things like default values and type casting, that this plugin could handle as well to give even more incentive to plugin authors to use it. It could also be used for automatic documentation in the future! I suspect it would also make the code smaller, enough that it would be worth including. Right now, it's still easier for most plugin authors to just do it themselves and avoid the perf overhead and syntactic awkardness. As it currently stands, I wouldn't use this as a plugin author I'm afraid :/

Here's an example of a settings spec I've seen in another project: https://github.com/haystack/flotr-extension/blob/master/src/scripts/bar-chart-view.js#L64-L85
We don't have to follow that format, but it's a good starting point.

zeitgeist87 · 2016-05-01T06:16:33Z

then the plugin would provide a method to get their values for a given element.

But the whole idea of the plugin is, that settings can be inherited from parent elements. So you can set your settings on the <body> tag and every code block inherits them.

This would avoid having to specially mark elements that contain data to avoid false matches, or the awkwardness and perf overhead of iterating over attributes.

Maybe I misunderstand you here, but the settings get parsed into a normal JavaScript object. Using it couldn't be easier. No awkwardness...

if (env.settings['left-trim']) {
    // do something
}

Default options are trivial:

assign({
    'left-trim': true,
    'right-trim': false,
    something: 80
}, env.settings);

Having the plugins provide a settings spec would also allow for cool things like default values and type casting, that this plugin could handle as well to give even more incentive to plugin authors to use it.

Boolean values are already typecast:

<!-- The result would be: env.settings['something'] = false -->
<div class="language-javascript no-something">
<!-- The result would be: env.settings['something'] = true -->
<code data-something></code>
<!-- The result would be: env.settings['something'] = true -->
<code data-something="true"></code>
<!-- The result would be: env.settings['something'] = false -->
<code data-something="false"></code>
</div>

I suspect it would also make the code smaller, enough that it would be worth including.

I don't see how a spec would make the code smaller. I still have to go through all the parent elements and parse all the attributes to compare them to the spec. Maybe I misunderstand your proposal here...

zeitgeist87 · 2016-05-01T07:13:17Z

Right now, it's still easier for most plugin authors to just do it themselves and avoid the perf overhead and syntactic awkardness.

I don't understand. It supports exactly the same syntax that most plugins use right now:

<pre class="language-javascript line-numbers"><code>
// JS code
</code></pre>

Here is what the line numbers plugin does right now:

    var pre = env.element.parentNode;
    var clsReg = /\s*\bline-numbers\b\s*/;
    if (
        !pre || !/pre/i.test(pre.nodeName) ||
            // Abort only if nor the <pre> nor the <code> have the class
        (!clsReg.test(pre.className) && !clsReg.test(env.element.className))
    ) {
        return;
    }

Here is how it would look with the parse settings plugin using the exact same HTML code:

var pre = env.element.parentNode;
if (!pre || !/pre/i.test(pre.nodeName) || !env.settings || !env.settings['line-numbers']) {
    return;
}

Not only that. You can also set line-numbers globally with the same syntax people are already used to:

<body class="language-javascript line-numbers">

<pre><code>
// JS code
</code></pre>

<pre><code>
// JS code
</code></pre>

<pre><code>
// JS code
</code></pre>

</body>

zeitgeist87 · 2016-05-01T07:25:58Z

You can also disable the global setting locally:

<body class="language-javascript line-numbers">

<pre><code>
// JS code
</code></pre>

<pre><code>
// JS code
</code></pre>

<pre class="no-line-numbers"><code>
// JS code
</code></pre>

<pre><code data-line-numbers="false">
// JS code
</code></pre>

</body>

For the plugin author nothing changes. The following code is still enough:

var pre = env.element.parentNode;
if (!pre || !/pre/i.test(pre.nodeName) || !env.settings || !env.settings['line-numbers']) {
    return;
}

LeaVerou · 2016-05-02T18:02:00Z

Only if you want settings on some unrelated tag you have to mark it

I don't see why you would want settings on anything other than a) an ancestor b) the element itself c) the script element. This is confusing and I'd advocate for getting rid of it.

For the plugin author nothing changes.

Yes, but we cannot treat every class and data attribute as a potential setting. It's just a lot of overhead, for little reason.

zeitgeist87 · 2016-05-02T19:24:26Z

Only if you want settings on some unrelated tag you have to mark it
I don't see why you would want settings on anything other than a) an ancestor b) the element itself c) the script element. This is confusing and I'd advocate for getting rid of it.

It always has to be an ancestor. Only ancestors are considered.

<!-- IS an ancestor, but has no Prism language class.
Therefore it needs the marking attribute data-prism -->
<body data-prism data-line-numbers="false">

<!-- does NOT work because it is not an ancestor -->
<div data-line-numbers></div>

<!-- no mark is needed because of the language-javascript class -->
<div class="language-javascript" data-line-numbers>

<!-- no mark is needed because it is the pre tag -->
<pre class="no-line-numbers"><code>
// JS code
</code></pre>

<pre><code>
// JS code
</code></pre>

</div>
</body>

I think the marking attribute is useful. Most of the time it can be avoided by placing the settings on the same tag as the language class, but it is still a nice option to have.

Nevertheless I can remove it if you dislike it that much 😄

For the plugin author nothing changes.
Yes, but we cannot treat every class and data attribute as a potential setting. It's just a lot of overhead, for little reason.

I disagree. Only the parent elements are scanned. How deep does an average DOM tree go? I would guess 10 at the most. So you have 10 elements that get immediately and efficiently filtered. All elements without a Prism language class or the marker attribute are thrown away. Most of the time this leaves only one or two elements. There is no reason for a user to put Prism settings on all 10 ancestors so we parse all attributes of 2 elements!

Currently every plugin parses the DOM for its settings. Basically the same operations are repeated for every plugin used. It is better to do it once for all plugins.

LeaVerou · 2016-05-02T20:14:04Z

I think the marking attribute is useful. Most of the time it can be avoided by placing the settings on the same tag as the language class, but it is still a nice option to have.

Ancestors should not need any kind of special marking to contain settings. Inheriting settings should be as easy as specifying them on the pre/code elements themselves, just like how language inheritance works. Yet another reason to have plugins declare their settings.

I disagree. Only the parent elements are scanned. How deep does an average DOM tree go? I would guess 10 at the most. So you have 10 elements that get immediately and efficiently filtered. All elements without a Prism language class or the marker attribute are thrown away. Most of the time this leaves only one or two elements. There is no reason for a user to put Prism settings on all 10 ancestors so we parse all attributes of 2 elements!

If you have plugins declare their settings this is not needed though. People use classes and data- attributes for all sorts of reasons. You will end up getting tons of pointless "settings" that will be passed to each plugin, whether they are relevant or not. It's not just a matter of how slow it is, it's also that it's doing unneeded work.

Currently every plugin parses the DOM for its settings. Basically the same operations are repeated for every plugin used. It is better to do it once for all plugins.

That's an implementation detail, not something that should affect the plugin's API. The plugin could combine all declared settings and do it in one pass.

Overall: I'm strongly opposed to parsing random classes and data attributes as settings just because they happen to be specified on a Prism element. I'm also strongly opposed to having to add attributes to ancestors to allow them the privilege of being able to specify settings, which is a side effect of parsing every random class and data attribute as a setting. If plugins declare their settings, all these issues go away at a very small price: Plugins just giving us a literal of all the settings they expect, which is something they often do anyway. That way, we could even take care of default values for them.

zeitgeist87 · 2016-05-02T21:00:36Z

Ok I give up. I will implement it with specs 😄

LeaVerou · 2016-05-02T22:09:39Z

Because I convinced you or because I said I'm strongly opposed to the alternative?
If it's the latter, we should discuss more.

Golmote · 2016-05-02T23:20:29Z

@LeaVerou Ok so plugins might declare their settings... But will it still be possible to override the plugin's defaults by setting classes or data- attributes on pre, code, or any ancestor?

LeaVerou · 2016-05-03T00:00:08Z

Yes, plugins will declare their settings and their defaults, then this plugin will be responsible for returning their actual settings to the calling plugin, after looking for these specific classes and/or attributes in the pre/code and its ancestors.

zeitgeist87 · 2016-05-03T05:27:17Z

Because I convinced you or because I said I'm strongly opposed to the alternative?

You've convinced me to give it a try. At first I thought, that it would only complicate things, but in your last comment you described your idea in more detail and I can see the advantages. Especially the fact, that settings can be on any ancestor is nice.

default values

zeitgeist87 · 2016-05-09T06:02:28Z

@LeaVerou I have implemented your suggestion. It works pretty well, but it needs slightly more code to do it. Maybe we can remove a few features to make it smaller...

LeaVerou · 2016-07-19T15:48:01Z

Nooooo, don't close it :(

zeitgeist87 mentioned this pull request Jan 7, 2016

Whitespace normalizer plugin #847

Merged

mAAdhaTTah mentioned this pull request Feb 13, 2016

Plugins: Toolbar & Copy to Clipboard #891

Merged

5 tasks

zeitgeist87 force-pushed the ParseSettings branch from 03c915a to 71b5541 Compare April 30, 2016 14:31

LeaVerou reviewed May 1, 2016
View reviewed changes

Optimize parse-settings plugin for code size

b5a5e9c

Implement plugin settings using specifications with type and

21ef141

default values

zeitgeist87 mentioned this pull request Jun 2, 2016

Whitespace normalizer has no way to opt out #964

Closed

zeitgeist87 closed this Jul 3, 2016

plasticrake mentioned this pull request Jun 9, 2022

Normalize Whitespace Plugin: add configuration via attributes #3467

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parse Settings plugin #851

Parse Settings plugin #851

zeitgeist87 commented Jan 7, 2016

zeitgeist87 commented Apr 30, 2016

Golmote commented Apr 30, 2016

LeaVerou May 1, 2016

zeitgeist87 May 1, 2016

LeaVerou commented May 1, 2016

zeitgeist87 commented May 1, 2016

zeitgeist87 commented May 1, 2016 •

edited

Loading

zeitgeist87 commented May 1, 2016

LeaVerou commented May 2, 2016

zeitgeist87 commented May 2, 2016

LeaVerou commented May 2, 2016

zeitgeist87 commented May 2, 2016

LeaVerou commented May 2, 2016

Golmote commented May 2, 2016

LeaVerou commented May 3, 2016

zeitgeist87 commented May 3, 2016

zeitgeist87 commented May 9, 2016

LeaVerou commented Jul 19, 2016

Parse Settings plugin #851

Parse Settings plugin #851

Conversation

zeitgeist87 commented Jan 7, 2016

zeitgeist87 commented Apr 30, 2016

Golmote commented Apr 30, 2016

LeaVerou May 1, 2016

Choose a reason for hiding this comment

zeitgeist87 May 1, 2016

Choose a reason for hiding this comment

LeaVerou commented May 1, 2016

zeitgeist87 commented May 1, 2016

zeitgeist87 commented May 1, 2016 • edited Loading

zeitgeist87 commented May 1, 2016

LeaVerou commented May 2, 2016

zeitgeist87 commented May 2, 2016

LeaVerou commented May 2, 2016

zeitgeist87 commented May 2, 2016

LeaVerou commented May 2, 2016

Golmote commented May 2, 2016

LeaVerou commented May 3, 2016

zeitgeist87 commented May 3, 2016

zeitgeist87 commented May 9, 2016

LeaVerou commented Jul 19, 2016

zeitgeist87 commented May 1, 2016 •

edited

Loading