Bug 1148743 - Add button to open samples in JSFiddle #3147

Closed
wants to merge 2 commits into
from

Projects

None yet

6 participants

@iakshay
Contributor
iakshay commented Mar 29, 2015

Code to insert buttons using javascript.

    $('.sample-code-frame, .sample-code-table').parent().prepend(function() {
        return '<div><a class="button open-in-host" data-host="jsfiddle" data-section="' + 
        $(this).children('iframe').attr('id').substring("frame_".length) + '">Open in JSFiddle</a>'
        + '<a style="margin-left:10px" class="button open-in-host" data-host="codepen" data-section="' + 
        $(this).children('iframe').attr('id').substring("frame_".length) + '">Open in Codepen</a></div>';
    });
@stephaniehobson
Contributor

We can add a line to forms.styl which adds the margin to any button after the first button and then we can localize it for right to left display as well.

.open-in-host + .open-in-host {
bidi-style(margin-left, ($grid-spacing / 2), margin-right, 0);
}

@darkwing darkwing and 1 other commented on an outdated diff Mar 30, 2015
media/js/wiki.js
@@ -847,5 +847,73 @@
if($('details').length){
initDetailsTags();
}
+
+ function create_jsfiddle(title, htmlCode, cssCode, jsCode) {
+ var $form = $('<form method="post" target="_blank" action="https://jsfiddle.net/api/post/library/pure/">'
+ + '<input type="hidden" id="html" name="html" />'
@darkwing
darkwing Mar 30, 2015 Member

Do we need any of these id attributes? These seem unnecessary. Instead of $form.find('#html'), can you do $form.find('input[name=html')?

@iakshay
iakshay Mar 31, 2015 Contributor

I thought id would be better performance-wise, although in this case since we have very few elements - it shouldn't make much difference.

@darkwing darkwing and 1 other commented on an outdated diff Mar 30, 2015
media/js/wiki.js
@@ -847,5 +847,73 @@
if($('details').length){
initDetailsTags();
}
+
+ function create_jsfiddle(title, htmlCode, cssCode, jsCode) {
+ var $form = $('<form method="post" target="_blank" action="https://jsfiddle.net/api/post/library/pure/">'
+ + '<input type="hidden" id="html" name="html" />'
+ + '<input type="hidden" id="css" name="css" />'
+ + '<input type="hidden" id="js" name="js" />'
+ + '<input type="hidden" id="panel_html" name="panel_html" value="0" />'
+ + '<input type="hidden" id="panel_css" name="panel_css" value="0" />'
+ + '<input type="hidden" id="panel_js name="panel_js value="0" />'
+ + '<input type="hidden" id="title" name="title" value="' + title +'" />'
@darkwing
darkwing Mar 30, 2015 Member

This seems unsafe because if the title includes ", that could become an injection problem. Should use val() like you do below.

@iakshay
iakshay Mar 31, 2015 Contributor

Fixed

@darkwing darkwing and 1 other commented on an outdated diff Mar 30, 2015
media/js/wiki.js
+
+ $('#wikiArticle').on('click', 'a.open-in-host', function(){
+ var $button = $(this);
+ var isDisabled = $button.attr('data-disabled');
+ var section = $button.attr('data-section');
+ var sampleCodeHost = $button.attr('data-host');
+
+ // track the click and sample code host
+ mdn.analytics.trackEvent({
+ category: 'Samples',
+ action: 'open-' + sampleCodeHost,
+ label: section
+ });
+
+ // disable the button, till we open the fiddle
+ if($button.hasClass('disabled')) return;
@darkwing
darkwing Mar 30, 2015 Member

Should this go above the event tracking code?

@iakshay
iakshay Mar 31, 2015 Contributor

You are right :)

@stephaniehobson stephaniehobson and 1 other commented on an outdated diff Mar 30, 2015
media/js/wiki.js
+ + '<input type="submit" />'
+ + '</form>');
+ var data = {'title': title, 'html': htmlCode, 'css': cssCode, 'js': jsCode};
+ $form.find('#data').val(JSON.stringify(data));
+ $form.submit();
+ }
+
+ function create_sample(sampleCodeHost, title, htmlCode, cssCode, jsCode) {
+ if(sampleCodeHost == 'jsfiddle') {
+ create_jsfiddle(title, htmlCode, cssCode, jsCode);
+ } else if(sampleCodeHost == 'codepen') {
+ create_codepen(title, htmlCode, cssCode, jsCode);
+ }
+ }
+
+ $('#wikiArticle').on('click', 'a.open-in-host', function(){
@stephaniehobson
stephaniehobson Mar 30, 2015 Contributor

Since these links will not work without javascript enabled we should use the HTML button element instead of links. (links without the href element cannot receive keyboard focus).

@iakshay
iakshay Mar 31, 2015 Contributor

<button> isn't allowed in macros. So if keep it as a link, we can add it using macros in future.

I'm switching to <button> for now.

@stephaniehobson stephaniehobson commented on an outdated diff Mar 30, 2015
media/js/wiki.js
+ var data = {'title': title, 'html': htmlCode, 'css': cssCode, 'js': jsCode};
+ $form.find('#data').val(JSON.stringify(data));
+ $form.submit();
+ }
+
+ function create_sample(sampleCodeHost, title, htmlCode, cssCode, jsCode) {
+ if(sampleCodeHost == 'jsfiddle') {
+ create_jsfiddle(title, htmlCode, cssCode, jsCode);
+ } else if(sampleCodeHost == 'codepen') {
+ create_codepen(title, htmlCode, cssCode, jsCode);
+ }
+ }
+
+ $('#wikiArticle').on('click', 'a.open-in-host', function(){
+ var $button = $(this);
+ var isDisabled = $button.attr('data-disabled');
@stephaniehobson
stephaniehobson Mar 30, 2015 Contributor

When we switch to using a <button> instead of a <a> we can also use the HTML attribute disabled instead of creating our own. The browser then disables the button for us and we don't need to worry about writing code to manage the disabled button, so you could remove line 902 as well :)

As a note, the disabled attribute is boolean, which means have to add and remove the attribute not switch between true and false.

@stephaniehobson
Contributor

Wanted to add: super excited to see a PR for this. It's a feature everyone has been talking about for a while!

@iakshay
Contributor
iakshay commented Mar 31, 2015

Good for another review :)

I also made minor change in EmbedLiveSample macro which I have posted here

@iakshay
Contributor
iakshay commented Mar 31, 2015

Do we want to insert the buttons using optimizely?

@groovecoder
Member

IIRC, @openjck wanted to use optimizely to change the order of the buttons. So we may not need to insert them via optimizely, but we may run an Optimizely experiment that re-orders them for 50% of viewers.

@openjck
Contributor
openjck commented Apr 2, 2015

Having had some time to think about it, I would recommend just measuring clicks with Google Analytics. Switching the order of the buttons with Optimizely (or switching between showing one and showing another) would be bad for habituation.

If one button is clicked way more than the other, we could decide to support only that service. If the buttons receive similar numbers of clicks, we could decide to support both services. I don't expect button placement will have a very significant effect on number of clicks.

edit: Ah, but of course Optimizely and Waffle bucket users only once and keep users in their buckets between reloads. (At least in theory. I notice that the current homepage heading Waffle doesn't persist buckets between reloads.) In that case, we could use Optimizely to show one button or the other and measure which one gets more clicks. In that case, inserting the buttons with Optimizely probably would be the easier approach. That would mitigate the effect of any FOUCs.

I don't expect the numbers will be significantly different, but it wouldn't hurt to try it out.

@groovecoder
Member

If you'd like to try it out, go for it. For now, @iakshay - could we put this behind a waffle flag so we can merge and push it while @openjck decides (or not) to try out Optimizely?

@openjck openjck was assigned by groovecoder Apr 2, 2015
@stephaniehobson stephaniehobson commented on the diff Apr 2, 2015
media/stylus/base/elements/forms.styl
@@ -114,3 +114,7 @@ label {
text-decoration: none;
}
}
+
+.open-in-host + .open-in-host {
+ bidi-style(margin-left, ($grid-spacing / 2), margin-right, 0);
+}
@stephaniehobson
stephaniehobson Apr 2, 2015 Contributor

Needs a line break at end of file (picky GitHub is picky).

@stephaniehobson
Contributor

All my comments have been addressed, thank you :) It would be nice if the sample code in the commit message was also updated.

@openjck
Contributor
openjck commented Apr 2, 2015

I'm definitely up for doing an A/B test. I'll coordinate with @iakshay offline to get it going.

@iakshay
Contributor
iakshay commented Apr 2, 2015

Updated code to insert buttons
updated again

function create_buttons() {
    var section = $(this).find('iframe').attr('id').substring("frame_".length);
    return '<div style="margin-bottom:10px;"><button class="open-in-host" data-host="jsfiddle" data-section="' + 
    section + '"><i aria-hidden="true" class="icon-jsfiddle"></i> Open in JSFiddle</i></button>'
    + '<button class="open-in-host" data-host="codepen" data-section="' + 
    section + '"><i aria-hidden="true" class="icon-codepen"></i> Open in Codepen</button></div>';
}

$('.sample-code-frame').parent().prepend(create_buttons);
$('.sample-code-table').before(create_buttons); 
@iakshay iakshay commented on the diff Apr 2, 2015
media/js/wiki.js
+
+ // track the click and sample code host
+ mdn.analytics.trackEvent({
+ category: 'Samples',
+ action: 'open-' + sampleCodeHost,
+ label: section
+ });
+
+ // disable the button, till we open the fiddle
+ $button.attr('disabled', 'disabled');
+ var sampleUrl = window.location.href.split('#')[0] + '?section=' + section + '&raw=1';
+ $.get(sampleUrl).then(function(sample) {
+ var $sample = $('<div />').append(sample);
+ var htmlCode = $sample.find('.brush\\:.html, .brush\\:.html\\;').text();
+ var cssCode = $sample.find('.brush\\:.css, .brush\\:.css\\;').text();
+ var jsCode = $sample.find('.brush\\:.js, .brush\\:.js\\;').text();
@iakshay
iakshay Apr 2, 2015 Contributor

@darkwing Will these selectors match all our code sample snippets?

I have written selectors for the following 2 variations

// Case 1
<pre class="brush: js"></pre>
// Case 2
<pre class="brush: js; highlight:[7]"></pre>
@iakshay
iakshay Apr 2, 2015 Contributor

Also, I'm not sure if converting code to base64 is necessary.

@openjck
openjck Apr 7, 2015 Contributor

I was wondering the same thing. What do you think, @darkwing?

@openjck
openjck Apr 8, 2015 Contributor

Will these selectors match all our code sample snippets?

12:09 <davidwalsh> I believe so
12:09 <davidwalsh> I think taht's right

@openjck openjck added not ready and removed not ready labels Apr 7, 2015
@openjck
Contributor
openjck commented Apr 8, 2015

How would you feel about writing the insertion code into the repository, so that we would run something like mdn.injectFiddleButton() and mdn.injectCodepenButton from Optimizely?

The code is non-trivial, so it makes sense for us to revision it. We also want to be sure that it stays synchronized with other changes happening in the codebase.

@openjck openjck added the not ready label Apr 8, 2015
@openjck
Contributor
openjck commented Apr 8, 2015

Maybe we could even add the buttons, but use display: none on them by default. That would be easier to maintain. We used the same approach in the A/B testing groundwork in #3142.

@iakshay
Contributor
iakshay commented Apr 8, 2015

I have created mdn.insertSampleButtons(sites). To insert the buttons you can refer this snippet

// insert JSFiddle button
mdn.insertSampleButtons(['jsfiddle']);
// insert JSFiddle and Codepen buttons
mdn.insertSampleButtons(['jsfiddle', 'codepen']);
// other order
mdn.insertSampleButtons(['codepen', 'jsfiddle']);

I can hide them by default, but this way you can experiment with order also :)

@openjck openjck removed the not ready label Apr 14, 2015
@openjck
Contributor
openjck commented Apr 15, 2015

What would be a good page to spot-check this on? I'm having trouble finding pages that use .sample-code-frame or .sample-code-table.

Since this involves the live samples system, which @darkwing has done the most work with, I think it would be great to get his opinion too.

@openjck openjck assigned darkwing and unassigned openjck Apr 15, 2015
@darkwing darkwing commented on the diff Apr 15, 2015
media/js/wiki.js
+ var sampleUrl = window.location.href.split('#')[0] + '?section=' + section + '&raw=1';
+ $.get(sampleUrl).then(function(sample) {
+ var $sample = $('<div />').append(sample);
+ var htmlCode = $sample.find('.brush\\:.html, .brush\\:.html\\;').text();
+ var cssCode = $sample.find('.brush\\:.css, .brush\\:.css\\;').text();
+ var jsCode = $sample.find('.brush\\:.js, .brush\\:.js\\;').text();
+ var title = $sample.find('h2[name=' + section + ']').text();
+
+ openSample(sampleCodeHost, title, htmlCode, cssCode, jsCode);
+
+ $button.removeAttr('disabled');
+ });
+ });
+
+ function createSampleButtons(section, sites) {
+ var buttons = '<div style="margin-bottom:10px;">';
@darkwing
darkwing Apr 15, 2015 Member

We shouldn't inline styles -- a CSS class should be created.

@darkwing darkwing commented on the diff Apr 15, 2015
media/js/wiki.js
@@ -847,5 +847,90 @@
if($('details').length){
initDetailsTags();
}
+
+ function openJSFiddle(title, htmlCode, cssCode, jsCode) {
+ var $form = $('<form method="post" target="_blank" action="https://jsfiddle.net/api/mdn/">'
@darkwing
darkwing Apr 15, 2015 Member

Instead of a large string, let's create the elements with JS. We retrieve most of them to set their values anyway.

@darkwing darkwing commented on the diff Apr 15, 2015
media/js/wiki.js
+ + '<input type="hidden" name="html" />'
+ + '<input type="hidden" name="css" />'
+ + '<input type="hidden" name="js" />'
+ + '<input type="hidden" name="title" />'
+ + '<input type="hidden" name="wrap" value="b" />'
+ + '<input type="submit" />'
+ + '</form>');
+ $form.find('input[name=html]').val(htmlCode);
+ $form.find('input[name=css]').val(cssCode);
+ $form.find('input[name=js]').val(jsCode);
+ $form.find('input[name=title]').val(title);
+ $form.submit();
+ }
+
+ function openCodepen(title, htmlCode, cssCode, jsCode) {
+ var $form = $('<form method="post" target="_blank" action="http://codepen.io/pen/define">'
@darkwing
darkwing Apr 15, 2015 Member

Same here -- let's create these elements programatically

@iakshay
iakshay Apr 19, 2015 Contributor

I think creating elements programatically will make the code too long and unreadable. But I'm happy to do it, if this breaks our style guide.

Also, should I move my code into separate file. IIRC, we want to waffle flag this feature as well as A/B test. Is this part as simple as creating separate js file, or do I need to do waffle flag checking in js also?

Let me know if there is anything else that needs to be fixed. I want this feature in production asap :)

@darkwing
darkwing Apr 29, 2015 Member

Let's waffle this, please

@openjck
openjck Apr 30, 2015 Contributor

Just to be sure, did you see that the buttons need to be manually inserted with JS?

@iakshay
iakshay Apr 30, 2015 Contributor

Would it be suffice to move the code to a new file for waffle flag or is there more to it? Can you link me to a commit that does something similar?

@iakshay
Contributor
iakshay commented Apr 15, 2015

@openjck - I modified the EmbedLiveSample macro to add those classes. Update macro is posted here

@groovecoder
Member

ping @darkwing - are you able to review this again?

@groovecoder
Member

@iakshay - are you able to waffle this?

@iakshay
Contributor
iakshay commented May 8, 2015

@groovecoder I'm not sure, how we waffle features client side. Do I just move the code to new file?

@groovecoder
Member

We use a django-waffle helper to create a window.waffle object. We use(d) it in demos/home.html and in ckeditor_config.js

@openjck openjck added the not ready label May 15, 2015
@openjck
Contributor
openjck commented May 15, 2015

Do we need to Waffle this? Even on this branch, the buttons need to be manually injected with JS.

@darkwing
Member

This does need to be waffled, yes.

@zalun
zalun commented Jun 9, 2015

any update?

@darkwing
Member
darkwing commented Jun 9, 2015

Superceded by: #3240

@darkwing darkwing closed this Jun 9, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment