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

Add ZeroClipboard to docs #12690

Merged
merged 6 commits into from
Jun 10, 2014
Merged

Add ZeroClipboard to docs #12690

merged 6 commits into from
Jun 10, 2014

Conversation

mdo
Copy link
Member

@mdo mdo commented Feb 11, 2014

Supersedes #12013.

This takes the work done by @soundarapandian, reorganizes it, replaces the ZeroClipboard script with the minified version, and restyles the copy button.

Definitely want @fat to peep this before it makes it's way into our docs.

@XhmikosR
Copy link
Member

We don't really need the minified js file since we minify everything ourselves :)

@cvrebert cvrebert added this to the v3.1.1 milestone Feb 11, 2014
@mdo
Copy link
Member Author

mdo commented Feb 11, 2014

Re: minified version—the fewer lines added to the source, the happier I am :).

@mdo mdo modified the milestones: v3.2.0, v3.1.1 Feb 11, 2014
@XhmikosR
Copy link
Member

LGTM. The previous button approach wasn't so bad either :)

@XhmikosR
Copy link
Member

@mdo: I have rebased this branch locally against the latest master and I split the grunt dist to a separate patch so that we can merge easily in the future if needed. Should I force push the branch?

@mdo
Copy link
Member Author

mdo commented Feb 28, 2014

@XhmikosR Yeah, if you'd like. I still need input on the JS stuff—it feels like it should be different.

@XhmikosR
Copy link
Member

Branch updated :)

About JS, it seems to work fine so far for me. Maybe @fat can chime in...

@hnrch02
Copy link
Collaborator

hnrch02 commented Feb 28, 2014

BTW, any news on when @fat will be back? It's kind of a bummer that our main JS guy hasn't been available for the last couple of months.

@XhmikosR
Copy link
Member

XhmikosR commented Mar 1, 2014

@mdo: I still don't get any error message without Flash installed...

var htmlBridge = $('#global-zeroclipboard-html-bridge')

// Handlers for ZeroClipboard
zeroClipboard.on('load', function(client) {
Copy link
Member

Choose a reason for hiding this comment

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

Isn't client unused here and in line 137 and 147?

Copy link
Member

Choose a reason for hiding this comment

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

yep

@mdo
Copy link
Member Author

mdo commented Mar 1, 2014

@XhmikosR I never hooked anything up for that, and I don't know that we need it. I think the only thing that should happen is if you have Flash, you shouldn't see the copy button. No false hopes that way :).

@XhmikosR
Copy link
Member

XhmikosR commented Mar 2, 2014

@mdo: I thought this line is supposed to do exactly that?

@soundarapandian
Copy link
Contributor

@XhmikosR I also tried to test the noflash case. But I did not get the error message in tooltip. I traced in the ZeroclibBoard plugin. For noflash case the browser still think like flash available. The exact line in the plugin is https://github.com/zeroclipboard/zeroclipboard/blob/master/ZeroClipboard.js#L447

I tested in Firefox without flash. The following code returns a valid object even though flash not available.

navigator.mimeTypes["application/x-shockwave-flash"]

Looks like this is issue with plugin. If someone confirm this we can report to zeroclipboard.

@XhmikosR
Copy link
Member

XhmikosR commented Mar 5, 2014

@soundarapandian: I'm not sure who's to blame here at all; I just saw that code which seemed to me that it is for checking if flash player is installed...

$('.highlight').each(function() {
var highlight = $(this)
var previous = highlight.prev()
var btnHtml = '<div class="zero-clipboard"><span class="btn-clipboard">Copy</span></div>'
Copy link
Member

Choose a reason for hiding this comment

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

I'd change this to $btn =$('<div class="zero-clipboard"><span class="btn-clipboard">Copy</span></div>')

that way you can just append the class below

@fat
Copy link
Member

fat commented Mar 17, 2014

sorry @hnrch02 was just a busy couple of months, trying to be a bit better

@hnrch02
Copy link
Collaborator

hnrch02 commented Mar 17, 2014

Who am I that you'd have to apologize to me—totally understandable and I'm glad that you're back! 🍻

@XhmikosR
Copy link
Member

@mdo @fat: rebased the PR against the latest master; removed the unused variables. I'll leave the rest of the changes to one of you two :)

@fat
Copy link
Member

fat commented Mar 17, 2014

still need to change btnHtml -> btn

@XhmikosR
Copy link
Member

Well, I say that just above :P

@XhmikosR
Copy link
Member

XhmikosR commented May 6, 2014

Rebased.

@XhmikosR
Copy link
Member

Ah, that last merge @mdo basically reverted my rebase :/

@XhmikosR
Copy link
Member

@mdo: I managed to clean up the branch. Instead of merging from master, better rebase before the final merge. :)

@cvrebert
Copy link
Collaborator

@fat Can we get a final thumbs-up on this?

@XhmikosR
Copy link
Member

XhmikosR commented Jun 5, 2014

Hmm. It seems more changes are needed for ZeroClipboard 2.0.0 which is out.

@cvrebert cvrebert modified the milestones: v3.2.1, v3.2.0 Jun 5, 2014
@XhmikosR
Copy link
Member

XhmikosR commented Jun 9, 2014

So... will someone try to update for ZeroClipboard 2.x, will we merge this as it is or ditch it?

Personally, I'd like this to be merged since I find it a very useful addition.

@mdo
Copy link
Member Author

mdo commented Jun 9, 2014

Yup, I'll get on the v2 thing soon.

@XhmikosR
Copy link
Member

@mdo: cool, thanks!

@XhmikosR
Copy link
Member

Actually, 2.0.3 seems to not support IE < 9. So I guess we should stick with 1.3.x.

zeroclipboard/zeroclipboard@50c4716

@cvrebert
Copy link
Collaborator

@XhmikosR Do we honestly care, so long as it doesn't fail in a way that breaks the page?

@mdo
Copy link
Member Author

mdo commented Jun 10, 2014

Unless the changes are sufficient to warrant it, I have no problem staying with the old version until we drop IE9 in v4 (though, maybe we don't do that anymore, I dunno).

@XhmikosR
Copy link
Member

@cvrebert: what @mdo says above. We better be consistent with what we support. Note that I personally don't care about IE < 9, I just feel we need to be consistent.

@cvrebert
Copy link
Collaborator

We're already a bit inconsistent on account of the Customizer, but whatever.

mdo added a commit that referenced this pull request Jun 10, 2014
@mdo mdo merged commit 0a3fe8c into master Jun 10, 2014
@mdo mdo deleted the docs_zeroclipboard branch June 10, 2014 06:36
@mdo mdo modified the milestones: v3.2.1, v3.2.0 Jun 10, 2014
@mdo mdo mentioned this pull request Jun 10, 2014
@soundarapandian
Copy link
Contributor

💚

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants