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

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

Closed
wants to merge 2 commits into from

Conversation

@iakshay
Copy link
Contributor

@iakshay 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>';
    });
@iakshay iakshay force-pushed the iakshay:open-jsfiddle branch from d8916cd to 6dda163 Mar 29, 2015
@stephaniehobson
Copy link
Contributor

@stephaniehobson stephaniehobson commented Mar 30, 2015

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);
}


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" />'

This comment has been minimized.

@darkwing

darkwing Mar 30, 2015
Contributor

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

This comment has been minimized.

@iakshay

iakshay Mar 31, 2015
Author 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.

+ '<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 +'" />'

This comment has been minimized.

@darkwing

darkwing Mar 30, 2015
Contributor

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

This comment has been minimized.

@iakshay

iakshay Mar 31, 2015
Author Contributor

Fixed

});

// disable the button, till we open the fiddle
if($button.hasClass('disabled')) return;

This comment has been minimized.

@darkwing

darkwing Mar 30, 2015
Contributor

Should this go above the event tracking code?

This comment has been minimized.

@iakshay

iakshay Mar 31, 2015
Author Contributor

You are right :)

}
}

$('#wikiArticle').on('click', 'a.open-in-host', function(){

This comment has been minimized.

@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).

This comment has been minimized.

@iakshay

iakshay Mar 31, 2015
Author 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.


$('#wikiArticle').on('click', 'a.open-in-host', function(){
var $button = $(this);
var isDisabled = $button.attr('data-disabled');

This comment has been minimized.

@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
Copy link
Contributor

@stephaniehobson stephaniehobson commented Mar 30, 2015

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

@iakshay iakshay force-pushed the iakshay:open-jsfiddle branch 2 times, most recently from 17ed6b2 to aad10d5 Mar 31, 2015
@iakshay
Copy link
Contributor Author

@iakshay iakshay commented Mar 31, 2015

Good for another review :)

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

@iakshay iakshay force-pushed the iakshay:open-jsfiddle branch from aad10d5 to 5206a27 Mar 31, 2015
@iakshay
Copy link
Contributor Author

@iakshay iakshay commented Mar 31, 2015

Do we want to insert the buttons using optimizely?

@iakshay iakshay force-pushed the iakshay:open-jsfiddle branch from 5206a27 to 852cc0c Apr 1, 2015
@groovecoder
Copy link
Contributor

@groovecoder groovecoder commented Apr 2, 2015

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
Copy link
Contributor

@openjck 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
Copy link
Contributor

@groovecoder groovecoder commented Apr 2, 2015

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?


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

This comment has been minimized.

@stephaniehobson

stephaniehobson Apr 2, 2015
Contributor

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

@stephaniehobson
Copy link
Contributor

@stephaniehobson stephaniehobson commented Apr 2, 2015

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

@openjck
Copy link
Contributor

@openjck 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
Copy link
Contributor Author

@iakshay 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 force-pushed the iakshay:open-jsfiddle branch from 852cc0c to 10d98f7 Apr 2, 2015
@iakshay iakshay force-pushed the iakshay:open-jsfiddle branch from 10d98f7 to 28217bf Apr 2, 2015
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();

This comment has been minimized.

@iakshay

iakshay Apr 2, 2015
Author 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>

This comment has been minimized.

@iakshay

iakshay Apr 2, 2015
Author Contributor

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

This comment has been minimized.

@openjck

openjck Apr 7, 2015
Contributor

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

This comment has been minimized.

This comment has been minimized.

@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
Copy link
Contributor

@openjck 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
Copy link
Contributor

@openjck 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
Copy link
Contributor Author

@iakshay 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
Copy link
Contributor

@openjck 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
});

function createSampleButtons(section, sites) {
var buttons = '<div style="margin-bottom:10px;">';

This comment has been minimized.

@darkwing

darkwing Apr 15, 2015
Contributor

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

@@ -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/">'

This comment has been minimized.

@darkwing

darkwing Apr 15, 2015
Contributor

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

}

function openCodepen(title, htmlCode, cssCode, jsCode) {
var $form = $('<form method="post" target="_blank" action="http://codepen.io/pen/define">'

This comment has been minimized.

@darkwing

darkwing Apr 15, 2015
Contributor

Same here -- let's create these elements programatically

This comment has been minimized.

@iakshay

iakshay Apr 19, 2015
Author 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 :)

This comment has been minimized.

@iakshay

iakshay Apr 21, 2015
Author Contributor

This comment has been minimized.

@darkwing

darkwing Apr 29, 2015
Contributor

Let's waffle this, please

This comment has been minimized.

@openjck

openjck Apr 30, 2015
Contributor

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

This comment has been minimized.

@iakshay

iakshay Apr 30, 2015
Author 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
Copy link
Contributor Author

@iakshay iakshay commented Apr 15, 2015

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

@groovecoder
Copy link
Contributor

@groovecoder groovecoder commented Apr 29, 2015

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

@groovecoder
Copy link
Contributor

@groovecoder groovecoder commented May 8, 2015

@iakshay - are you able to waffle this?

@iakshay
Copy link
Contributor Author

@iakshay 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
Copy link
Contributor

@groovecoder groovecoder commented May 11, 2015

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
Copy link
Contributor

@openjck 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
Copy link
Contributor

@darkwing darkwing commented May 27, 2015

This does need to be waffled, yes.

@zalun
Copy link

@zalun zalun commented Jun 9, 2015

any update?

@darkwing
Copy link
Contributor

@darkwing 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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.