Skip to content

Conversation

@cherifGsoul
Copy link
Contributor

For canjs/bit-docs-html-canjs#561

This adds Run button to run code in CodePen, the logic behind the changes:

  • Remove all CodePen links
  • Register a PrismJS toolbar button

I upgraded Zombie in order to use modern browser functions.

index.js Outdated
el = parents[i];
break;
}
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Does registerButton only need to be called once (vs. a call for each instance of the toolbar)?

Instead of keeping the parents, should we walk up from env.element to find the pre?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The registerButton needs to be called once otherwise it register all the buttons for each of the codes.

For parents I did try to walk up but it didn't work.

test.js Outdated
Array.from(codePens).forEach(function(codePen) {
codePen.click();
var toolbars = doc.querySelectorAll('.toolbar');
Array.from(toolbars).forEach(function(toolbar) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is Array.from necessary? I think toolbars.forEach would exist.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No I just changed the existed code that used it, I'll update it

index.js Outdated
if (hasRunBtn) {
var btn = document.createElement("button");
btn.innerHTML = "Run";
btn.addEventListener('click', function() {
Copy link
Member

Choose a reason for hiding this comment

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

do we need to setup this event handler for every codepen? Could event delegation be used to help peformance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@justinbmeyer updated, thank you!

@phillipskevin
Copy link

@cherifGsoul is this good to go?

@cherifGsoul
Copy link
Contributor Author

@cherifGsoul is this good to go?

I think so, @chasenlehara do you want to take a look before merging it?

index.js Outdated
while(el && el.parentNode) {
if(matches.call(el.parentNode, '.demo_wrapper')) {
var demoWrapper = el.parentNode;
return demoWrapper;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can just return demoWrapper instead of assigning it to a variable.

index.js Outdated
if (hasRunBtn) {
var btn = document.createElement("button");
btn.innerHTML = "Run";
document.body.addEventListener('click', function (ev) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This event listener only gets added once, right? It doesn’t get added for each button?

[I see the event delegation, which is nice, but I want to double check that the listener isn’t added multiple times. If it is, this logic should be moved outside the registerButton call.]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chasenlehara Fixed, thank you!

@cherifGsoul cherifGsoul merged commit 218d9c2 into master Aug 16, 2019
@cherifGsoul cherifGsoul deleted the prismjs-run-button branch August 16, 2019 19:12
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