Skip to content
This repository has been archived by the owner on Feb 16, 2021. It is now read-only.

Take element's scroll position into account when positioning html bridge #82

Merged
merged 3 commits into from Jan 18, 2013

Conversation

jrolfs
Copy link
Contributor

@jrolfs jrolfs commented Jan 18, 2013

No description provided.

@jrolfs
Copy link
Contributor Author

jrolfs commented Jan 18, 2013

I didn't see any tests for positioning. I can add them if deemed necessary.

@JamesMGreene
Copy link
Member

I know @jonrohan wanted to add some positioning tests but you are correct that there aren't any currently. NodeUnit + JSDOM can't really give us what we need there, so we'll have to add PhantomJS to the testing tools for it.

@jrolfs
Copy link
Contributor Author

jrolfs commented Jan 18, 2013

I had a feeling that was the case but didn't have time to look into it.

@jonrohan
Copy link
Contributor

Yeah, our current jsdom tests don't actually have any style or location data. which sucks a bit. so we don't have any tests.

This looks good to me, I'd just say you should run make if you can, to re-build the compiled .js source.

@jrolfs
Copy link
Contributor Author

jrolfs commented Jan 18, 2013

Compiled.

@jonrohan
Copy link
Contributor

doh, now there's conflicts probably in ZeroClipboard.js and ZeroClipboard.min.js because of the last pr I just merged.

@jrolfs
Copy link
Contributor Author

jrolfs commented Jan 18, 2013

Should I merge master and recompile?

@jonrohan
Copy link
Contributor

yeah give that a shot.

@jrolfs
Copy link
Contributor Author

jrolfs commented Jan 18, 2013

Okay, now try?

jonrohan added a commit that referenced this pull request Jan 18, 2013
Take element's scroll position into account when positioning html bridge
@jonrohan jonrohan merged commit e419c3c into zeroclipboard:master Jan 18, 2013
@jonrohan
Copy link
Contributor

excellent

@3rd-Eden
Copy link

3rd-Eden commented Feb 5, 2013

Any problems with a pull request that reverts this "fix" as it's breaking code as expressed in #93 and it doesn't include tests anyways.

@jonrohan
Copy link
Contributor

jonrohan commented Feb 5, 2013

fine by me

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

Successfully merging this pull request may close these issues.

None yet

4 participants