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

Plugins: Toolbar & Copy to Clipboard #891

Merged

Conversation

mAAdhaTTah
Copy link
Member

@mAAdhaTTah mAAdhaTTah commented Feb 13, 2016

I'm using Prism as the syntax highlighter for my WordPress plugin, WP-Gistpen. I've built a toolbar plugin that I'd like to contribute to Prism. I've got a first pass at extracting the plugin from its CommonJS origins,, but I need some feedback on how to change it to fit the Prism way of doing things, e.g. the HTML API. I also see some possible synergy with the Parse Settings plugin.

The Toolbar exposes two methods: a hook method, which is registered with Prism's complete hook, and a registerButton method, which takes a callback that creates the buttons. The callback will be passed the env and should return an a or span element, which other plugins can use to register themselves with the Toolbar to get their text or link to display.

I've also created a Clipboard plugin is an example of this callback, which creates an anchor tag that, when clicked, copies the code to the clipboard. I plan to extract that into a Prism plugin as well, but I'm not sure how Prism wants to handle dependencies like Clipboard.js, or how the download page will work with internal dependencies like this. In the interest of user friendliness, what's the best approach to take here?

Prior art:

Open Questions:

This plugin exposes a `registerButton` method, which other
plugins can use to add buttons to the toolbar. Comes with
styles.
Registers a "Hello World!" tag with the toolbar.
@mAAdhaTTah mAAdhaTTah changed the title Plugin: Toolbar & Copy to Clipboard Plugins: Toolbar & Copy to Clipboard Feb 13, 2016
@mAAdhaTTah
Copy link
Member Author

@LeaVerou @Golmote @zeitgeist87 I'm on vacation this coming week, so I should have some time to tweak this and extract the other plugin to your preferences, if you have guys have a chance to take a look and let me know what you think.

Thanks!

@zeitgeist87
Copy link
Collaborator

Hi @mAAdhaTTah,

I've tested your plugin and it works fine. I have a few comments on the plugin itself, but I can't really help you with your question about third-party dependencies.

  • Why do you need to expose the hook method? Is there a need to execute the hook manually?
  • You could additionally to registerButton provide a higher level method to create a button. Something like: createButton(text, onClick)
  • I think the helloworld button should do something. Maybe something like alert('Hello World!');
  • You could also create a useful demo button, that selects all of the code in the box: http://stackoverflow.com/questions/11128130/select-text-in-javascript
  • The CSS styling looks a bit boring, but that is just my opinion

Thanks for contributing to Prism! Sorry for the late response.

@mAAdhaTTah
Copy link
Member Author

Why do you need to expose the hook method? Is there a need to execute the hook manually?

It was mostly to expose the function in case a plugin/developer wanted to unhook and possibly run manually. Not sure if it's necessary, but figured the extra flexibility would be helpful down the line.

You could additionally to registerButton provide a higher level method to create a button. Something like: createButton(text, onClick)

This is actually a good idea. What about making the registerButton method polymorphic:

function registerButton(opts) {
  var callback;

  if (typeof opts === 'function') {
    callback = opts;
  } else {
    callback = function(env) {
      // create element from opts.text & optional opts.onClick
    };
  }
  callbacks.push(callback);
}

I would probably set this up to ensure the env is passed into the onClick method as well.

I think the helloworld button should do something. Maybe something like alert('Hello World!');

Can do. And if we like the polymorphic idea, we can use the helloworld button to demonstrate how to do it that way.

You could also create a useful demo button, that selects all of the code in the box: http://stackoverflow.com/questions/11128130/select-text-in-javascript

Interesting. Might be more useful as an example than the helloworld button. The Clipboard.js implementation uses "select-all" as a fallback when the actual copying fails (primarily in Safari, maybe older browsers).

The CSS styling looks a bit boring, but that is just my opinion

I mostly copied it from the original implementation with some tweaks (I preferred having each button appear visually distinct), but I am not really a designer, so if anyone has any improvements, I'm all ears.

Thanks for contributing to Prism! Sorry for the late response.

Not a problem! Figured it was worth following up, since you guys are pretty active. I've added checkboxes for the open questions remaining on this PR. If the polymorphic idea is acceptable, I should be able to make those changes tonight.

@zeitgeist87
Copy link
Collaborator

This is actually a good idea. What about making the registerButton method polymorphic:

I like that idea. It also allows later additions to the interface. Maybe someone would like to implement buttons with icons or something else...

@Golmote
Copy link
Contributor

Golmote commented Feb 22, 2016

Regarding third party scripts, I think the plugin page should simply mention explicitly that script is required. And that's it. ^^ I think Prism should not distribute directly the third party script.

@mAAdhaTTah
Copy link
Member Author

@Golmote Any issues/considerations for Node/Browserify users? I don't know if that matters on the Node side, but Browserify users, I believe, will have to explicitly assign the Clipboard var as a global. I'll have to take a closer look myself, but would a require check and call be acceptable? And would we be ok including it as a dependency in package.json?

@Golmote
Copy link
Contributor

Golmote commented Feb 22, 2016

I really don't feel like including it as a dependency of Prism globally.
Don't know much about Browserify so I really can't tell.

@LeaVerou
Copy link
Member

Yeah, I would be opposed to adding global dependencies to support a plugin.

However, I'm not sure about just mentioning that Clipboard.js is needed. Part of why Prism is so easy to use is that you download a copy and it just works. I wonder if we can use gulp to pull in the latest version from clipboard.js’ repo and bundle it with the plugin.
Alternatively, the plugin could load the CDN version of clipboard.js dynamically.

@mAAdhaTTah
Copy link
Member Author

There is an optionalDependencies key, but it doesn't appear to function differently from regular dependencies. I'll investigate some options and report back.

This allows developers to provide either a callback or an object
with a `text` string and an optional `onClick` function to create
a new button.
@mAAdhaTTah mAAdhaTTah force-pushed the plugin/toolbar-and-copy-to-clipboard branch from 6af7086 to 44d0c1d Compare February 24, 2016 05:20
@mAAdhaTTah
Copy link
Member Author

So optionalDependencies would mean that the dependency gets installed when installed from npm, but it's not required, so if it's not found or something, the install would still work. I don't know if that's acceptable, but one option is to add Clipboard.js as an optionalDependency. Presumably, if you're installing from npm, you have a build process (likely Browserify/Webpack), which should work with the require check I added (though I still need to fully test that).

Alternatively:

I wonder if we can use gulp to pull in the latest version from clipboard.js’ repo and bundle it with the plugin.

This should be possible; maybe we could do something similar to the way the autoloader's language dependencies map is built. It would mean that the Clipboard.js library would be inlined into the Copy to Clipboard plugin.

Alternatively, the plugin could load the CDN version of clipboard.js dynamically.

The only issue with this is we can't be sure the CDN asset will be fully loaded before the highlighting starts, unless DOMContentLoaded will only fire after all assets (including async ones) are loaded.

The only other idea I had was to modify the downloads page and ensure Clipboard.js is loaded first there when selecting the Copy to Clipboard plugin, but I'm not sure how to accomplish that at first glance of the downloads.js file.

I've pushed a first pass at Copy to Clipboard plugin. The demo page has Clipboard.js loaded from the CDN. Definitely interested in your feedback.

if (typeof opts.onClick === 'function') {
element = document.createElement('a');
element.addEventListener('click', function () {
opts.onClick(env);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would also pass the this pointer like this:

opts.onClick.call(this, env, opts);

That way the callback function gets access to the element that was clicked.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know how important it is, but I didn't want to change the this value of the passed in callback in case the developer is relying on it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well without it, the this pointer will point to the opts object and the developer has no access to the created DOM element. I think it is standard practice to set the this pointer to something sensible, like the source of the click event. In what specific use case would the developer want this to be the opts object?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess it was more about not messing with developer expectations, but I think you're right. Will update.

@zeitgeist87
Copy link
Collaborator

The design on the https://clipboardjs.com/ homepage looks nice. They have nice copy buttons for their source code samples. Nice fade-in and fade-out animations, nice icon, colored box-shadows and small transparent speech bubbles with status messages. We could use that design as an inspiration.

This ensures additional HTML can't be passed to the toolbar
via the `text` property, ensuring a consistent display for the
buttons.
This provides access to the clicked element, which is what `this`
is usually bound to on event listeners.
This will install Clipboard.js when installing from `npm`, but
won't fail the build if the installation of Clipboard.js fails.
@mAAdhaTTah
Copy link
Member Author

Added a hover animation and a drop shadown. Does that make it pop a little bit more?

Also, I added Clipboard.js to optionalDependencies, but if that's not something you want, I can remove that commit. Even with that added, the download page doesn't bundle with Clipboard.js. Should it?

* gh-pages: (22 commits)
  Remove direction-property from themes
  Add tests for new greedy-pattern feature and fix bug in Kotlin
  Fix double HTML-encoding bug in Groovy language
  Add comments to better document the greedy-pattern feature
  Partial solution for the "Comment-like substrings"-problem
  Add property 'aliasTitles' to components.js
  [unescaped markup] Fix small issues @zeitgeist87 pointed out
  [unescaped markup] Fixed bug with escaped </script>
  Added unescaped markup plugin (hidden)
  Implemented @zeitgeist87’s suggestion in PrismJS#890 re: env.elements
  Changed weight in the header, as we’re now 2KB minified & gzipped. Also way overdue :)
  Changed the text in the header. Way overdue, as Prism’s popularity has way surpassed that of Dabblet
  Add before-highlightall hook
  Fix catastrophic backtracking regex issues
  Add missing prism.js to the documentation of normalize-whitespace
  Cleanup normalize-whitespace and improve keep-markup integration
  Preserve Markup in Normalize-Whitespace plugin
  Update CHANGELOG and run gulp
  Removed firstWhiteSpaces code
  Add support for automatic line breaks
  ...

# Conflicts:
#	components.js
@mAAdhaTTah
Copy link
Member Author

Bump. Thoughts on handling the dependency?

@LeaVerou
Copy link
Member

What about checking if Clipboard.js is loaded (e.g. via some global it defines) and if not, loading it dynamically from the CDN? That way we don't have the issues of bundling a possibly outdated version or the hassle of requiring users to load it themselves.

var element;

if (typeof opts.onClick === 'function') {
element = document.createElement('a');
Copy link
Contributor

@Golmote Golmote Oct 9, 2016

Choose a reason for hiding this comment

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

We should probably use a <button type="button"> element here, instead of an <a>.

Or we could allow for both depending on whether there is an onClick property (button) or an href property (link).

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do. I used the a b/c there would be fewer style overrides, as browsers tend to add a number of default styles to buttons, but it would be more semantically correct.

@Golmote
Copy link
Contributor

Golmote commented Oct 9, 2016

I think there should be a way to reorder the labels/actions in the toolbar, especially since some of them may be added automatically by plugins, and so depend on the order in which the plugins executed.

There could be a way to add buttons with the HTML API by providing a data-label-onclick attribute that takes the name of a global function?

There should also be a way to add multiple labels, maybe with numbered attributes, like data-label-1, data-label-2, etc.

<script>
function version_info() { alert('Some info about the version'); };
</script>
<pre data-label-1="v2.8" data-label-1-onclick="version_info" data-label-2="Download lib" data-label-2-href="http://example.com/lib.js">

I'm still very bad at naming things, so there are really just suggestions...

@mAAdhaTTah
Copy link
Member Author

That sounds interesting, but how do we coordinate between the HTML API & the JS API? Copy to Clipboard isn't using the HTML API; should it just go second, all of the HTML-based labels/buttons have been added?

Regarding the data-label-onclick, if you're going to have to write JavaScript anyway, I think I'd rather push the user to use the JS API than reference a global function. Providing a url via HTML attributes would be useful though.

@Golmote
Copy link
Contributor

Golmote commented Oct 9, 2016

You're right the data-label-onclick is quite useless.

Regarding the coordination, I think there should be a way to position the buttons that were added through plugins. Maybe this could be achieved with other data attributes

Also adds a `url` property which creats an anchor tag and sets
the href. Adds some styles to override the button defaults.
This allows the HTML API to create links in the Toolbar.
@mAAdhaTTah
Copy link
Member Author

I fixed the a -> button and added support for data-url to add a link instead of just a plain label. I was looking into managing multiple labels, and I'm beginning to feel that API might get ugly & unwieldy. I think if you need multiple labels/buttons, you should be doing them in JS.

The HTML API provides a simple API for adding a single label. If you need multiple labels, or onClick handlers, or anything else more complex, I think we should push users to use the JS API.

@Golmote
Copy link
Contributor

Golmote commented Oct 9, 2016

I'm fine with it. If there is a real need for multiple labels through HTML API, it might be added later anyway.

What about the order, when using plugins? Can you think of a way to handle this using HTML?

@mAAdhaTTah
Copy link
Member Author

We could ask plugins to register by name, then have users declare on a the html or body the order they should appear, e.g.:

<html data-toolbar-order="['label', 'clipboard', 'language']">

We can fall back to the order they were registered if the data attribute doesn't exist.

@Golmote
Copy link
Contributor

Golmote commented Oct 9, 2016

It seems good. Though I don't think there's a need for the brackets and single quotes. A comma separated list of aliases should be enough.

Would it be possible to control the position of the label added through the data-label attribute this way? Maybe by giving it an arbitrary alias?

@mAAdhaTTah
Copy link
Member Author

Would it be possible to control the position of the label added through the data-label attribute this way? Maybe by giving it an arbitrary alias?

In the above example, label was supposed to refer to the position of the data-label attribute. Maybe toolbar is more explicit?

@Golmote
Copy link
Contributor

Golmote commented Oct 9, 2016

Oh no, sorry, "label" is fine! I missed it somehow.

The plugins should probably register their exact name for consistency, i.e. "show-language".

@LeaVerou
Copy link
Member

Idea: What about enabling data-label to link to a <template> element containing arbitrary HTML? Then people can have custom buttons and everything in that, with their own event handlers.
Of course it shouldn't be the only way to specify a label, because having to include a separate <template> element if all you want is a textual label is kind of annoying.

Uses a data-attribute on the `body` tag to update the order,
should the user choose to do so.
@mAAdhaTTah
Copy link
Member Author

mAAdhaTTah commented Oct 16, 2016

Just updated to control the order via data-attribute.

@LeaVerou I'll take a look at that later today. Would be useful if someone wanted to include an icon or something of that nature. How would someone declare their event handlers in a <template>? Unless you just mean inline JS, e.g:

<template id="my-custom-button">
    <button onclick="alert('Hi')">Hello!</button>
</template>

@mAAdhaTTah
Copy link
Member Author

If we wanted to go crazy w/ that idea, we could even implement a mini-DSL and allow people to slot in the code snippet's language and such into the label:

<template id="my-custom-button">
    <button onclick="alert('Hi')">Hello {{language}}!</button>
</template>

@LeaVerou
Copy link
Member

How would someone declare their event handlers in a <template>?

Either inline or through event delegation.

This provides one of several options a user can implement in order to
get a custom button.

Also fixes some bugs in the documentation.
@mAAdhaTTah mAAdhaTTah force-pushed the plugin/toolbar-and-copy-to-clipboard branch from e471398 to 87e2aeb Compare October 24, 2016 14:55
@mAAdhaTTah
Copy link
Member Author

@LeaVerou PR updated to include that suggestion.

I think that addresses all of the questions I had when I opened the PR originally. I know this is a big one, and I just want to say thank you for everyone's patience and feedback in working through this.

@mAAdhaTTah
Copy link
Member Author

Will have some time this weekend to make edits as needed. Bumping this PR; maybe get a merge?

* gh-pages:
  Remove important token in ini definition (PrismJS#1047)
  Add missing `from` keyword to typescript & set `ts` as alias. (PrismJS#1042)
  Fix greedy-flag bug
  Add yarn.lock (PrismJS#1035)
  update patterns (PrismJS#1032)
  Test suite: fixed missing diff in error message
The autoloader will rehighlight the element after the language arrives.
This means the complete hook can run multiple times. Without a check,
multiple toolbars can get added to an element.
@Golmote
Copy link
Contributor

Golmote commented Nov 9, 2016

Sorry for the late answer. I'm merging now.

@Golmote Golmote merged commit 07b81ac into PrismJS:gh-pages Nov 9, 2016
thesave added a commit to thesave/prism that referenced this pull request Nov 9, 2016
* PrismJS/gh-pages:
  Plugins: Toolbar & Copy to Clipboard (PrismJS#891)
  Ini: Fix test after  PrismJS#1047
  Add support for the Jolie language (PrismJS#1014)
  Fix order of decoding entities in groovy (PrismJS#1049) (PrismJS#1050)
  Ruby: Make strings greedy. Fixes PrismJS#1048
  Ini: Remove newline at end of minified file.
  Ruby: Fix test after PrismJS#1023
  Remove important token in ini definition (PrismJS#1047)
  Add missing `from` keyword to typescript & set `ts` as alias. (PrismJS#1042)
  Fix greedy-flag bug
  Add yarn.lock (PrismJS#1035)
@mAAdhaTTah
Copy link
Member Author

No worries! Happy to contribute.

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.

4 participants