Issue 8615139: adblockpluschrome: Open share page in lightbox (Closed)
|
Created:
Oct. 23, 2012, 4:12 p.m. by Felix Dahlke Modified:
Oct. 25, 2012, 1:44 p.m. Reviewers:
Wladimir Palant Visibility:
Public. |
DescriptionI have changed the first run page to open the share page in a lightbox. I'm using fancyBox for this: http://fancyapps.com/fancybox/
Patch Set 1 #
Total comments: 4
Patch Set 2 : #
Total comments: 8
Patch Set 3 : #
Total comments: 14
Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #
Total comments: 1
MessagesTotal messages: 13
This is the code for what I've demonstrated today.
Main issue is that fancybox license isn't GPL-compatible and we probably want to release ABP Chrome under GPL on Friday. Plus: that's a whole lot more files than required. I have the impression that only one JavaScript and one CSS file are really necessary, these can be put into the jquery-ui directory. http://codereview.adblockplus.org/8615139/diff/1/firstRun.js File firstRun.js (right): http://codereview.adblockplus.org/8615139/diff/1/firstRun.js#newcode9 firstRun.js:9: href: getDocLink(network), We need to pass the variant parameter as well, otherwise we won't be able to track how the different variants of the first-run page perform. http://codereview.adblockplus.org/8615139/diff/1/firstRun.js#newcode27 firstRun.js:27: links[i].onclick = openSharePopup.bind(undefined, network); Did I mention already that I don't like onfoo? ;) Note that we should probably set href="#" and call preventDefault() on the event - this makes sure that the styles are consistent and we don't have to put them into firstRun.css explicitly.
I've addressed all issues and uploaded a new changeset. I've implemented a simple lightbox instead of using any third party ones - the only one that looks the way I want it and supports iframes is fancybox. Please note that I've used quite a bit of jQuery there, assuming that this will reduce issues with old/odd browsers in the future. But we could also stick to standard ECMAScript and use a polyfill instead. http://codereview.adblockplus.org/8615139/diff/1/firstRun.js File firstRun.js (right): http://codereview.adblockplus.org/8615139/diff/1/firstRun.js#newcode9 firstRun.js:9: href: getDocLink(network), On 2012/10/24 09:02:26, Wladimir Palant wrote: > We need to pass the variant parameter as well, otherwise we won't be able to > track how the different variants of the first-run page perform. Done. http://codereview.adblockplus.org/8615139/diff/1/firstRun.js#newcode27 firstRun.js:27: links[i].onclick = openSharePopup.bind(undefined, network); On 2012/10/24 09:02:26, Wladimir Palant wrote: > Did I mention already that I don't like onfoo? ;) > Note that we should probably set href="#" and call preventDefault() on the event > - this makes sure that the styles are consistent and we don't have to put them > into firstRun.css explicitly. You're right, my bad.
http://codereview.adblockplus.org/8615139/diff/12001/firstRun.js File firstRun.js (right): http://codereview.adblockplus.org/8615139/diff/12001/firstRun.js#newcode39 firstRun.js:39: glassPane.fadeIn(200, function() I would have loved to use Function.bind here, but the alternatives are: Function.prototype.bind.call(body.append, body, iframe); document.body.appendChild.bind(document.body, iframe[0]) I think what I have uploaded is more readable than any of those.
I don't really see a compelling reason to use jQuery here. Here an example using
CSS transitions just to make it clear what I meant in my comments:
<html>
<head>
<meta charset="utf-8" />
<style>
#glass
{
position: absolute;
left: 0px;
right: 0px;
top: 0px;
bottom: 0px;
background-color: black;
visibility: collapse;
opacity: 0;
transition: all .25s ease-in-out;
-webkit-transition: all .25s ease-in-out;
}
#glass.visible
{
visibility: visible;
opacity: 0.7;
}
</style>
<script>
window.setTimeout(function()
{
document.getElementById("glass").classList.add("visible");
window.setTimeout(function()
{
document.getElementById("glass").classList.remove("visible");
}, 2000);
}, 2000);
</script>
</head>
<body>
Some text
<div id="glass"></div>
</body>
</html>
http://codereview.adblockplus.org/8615139/diff/12001/firstRun.js
File firstRun.js (right):
http://codereview.adblockplus.org/8615139/diff/12001/firstRun.js#newcode33
firstRun.js:33: glassPane.fadeOut(200, glassPane.remove);
I think that using CSS transitions for fade-in/fade-out is a cleaner solution...
http://codereview.adblockplus.org/8615139/diff/12001/firstRun.js#newcode35
firstRun.js:35: });
Generally I'm trying to avoid creating non-trivial elements dynamically, for
sake of proper markup separation. Why not put this frame into firstRun.html with
attribute hidden="true"? Then you can simply remove that attribute whenever
necessary. Same goes for the glass pane of course.
http://codereview.adblockplus.org/8615139/diff/12001/firstRun.js#newcode39
firstRun.js:39: glassPane.fadeIn(200, function()
On 2012/10/24 14:57:33, Felix H. Dahlke wrote:
> I think what I have uploaded is more readable than any of those.
I agree - bind() is good for the obvious cases, this isn't one of them.
http://codereview.adblockplus.org/8615139/diff/12001/firstRun.js#newcode53
firstRun.js:53: .attr("href", "#")
Why does this attribute need to be defined dynamically?
http://codereview.adblockplus.org/8615139/diff/12001/skin/firstRun.css
File skin/firstRun.css (right):
http://codereview.adblockplus.org/8615139/diff/12001/skin/firstRun.css#newcod...
skin/firstRun.css:123: height: 100%;
I think that percentage width/height are a bit problematic in HTML, they depend
on the parent element. Maybe replace by bottom: 0 and right: 0?
http://codereview.adblockplus.org/8615139/diff/12001/skin/firstRun.css#newcod...
skin/firstRun.css:131: height: 550px;
A hardcoded size? Is it guaranteed to match pop-up contents?
> http://codereview.adblockplus.org/8615139/diff/12001/firstRun.js > File firstRun.js (right): > > http://codereview.adblockplus.org/8615139/diff/12001/firstRun.js#newcode33 > firstRun.js:33: glassPane.fadeOut(200, glassPane.remove); > I think that using CSS transitions for fade-in/fade-out is a cleaner solution... I'm aware of CSS transitions, but I've opted for jQuery's animation system because it seems we have to deal with old browsers in the future, down to IE6. Maybe we can find a CSS3 polyfill that does the job, but I think jQuery animations are the most proven approach. I don't mind using CSS transitions if you think that's not going to be a problem for us. > http://codereview.adblockplus.org/8615139/diff/12001/firstRun.js#newcode35 > firstRun.js:35: }); > Generally I'm trying to avoid creating non-trivial elements dynamically, for > sake of proper markup separation. Why not put this frame into firstRun.html with > attribute hidden="true"? Then you can simply remove that attribute whenever > necessary. Same goes for the glass pane of course. Sure, I can do that. > http://codereview.adblockplus.org/8615139/diff/12001/firstRun.js#newcode53 > firstRun.js:53: .attr("href", "#") > Why does this attribute need to be defined dynamically? Done. > http://codereview.adblockplus.org/8615139/diff/12001/skin/firstRun.css > File skin/firstRun.css (right): > > http://codereview.adblockplus.org/8615139/diff/12001/skin/firstRun.css#newcod... > skin/firstRun.css:123: height: 100%; > I think that percentage width/height are a bit problematic in HTML, they depend > on the parent element. Maybe replace by bottom: 0 and right: 0? Done. > http://codereview.adblockplus.org/8615139/diff/12001/skin/firstRun.css#newcod... > skin/firstRun.css:131: height: 550px; > A hardcoded size? Is it guaranteed to match pop-up contents? True, I'll use the size of the frame's content instead.
Uploaded a new change set. I've addressed the issues and am not using jQuery anymore. http://codereview.adblockplus.org/8615139/diff/12001/skin/firstRun.css File skin/firstRun.css (right): http://codereview.adblockplus.org/8615139/diff/12001/skin/firstRun.css#newcod... skin/firstRun.css:131: height: 550px; On 2012/10/25 07:12:00, Wladimir Palant wrote: > A hardcoded size? Is it guaranteed to match pop-up contents? Too bad, seems like I can't use iframe.contentDocument in Chrome unless the frame content is loaded from the same domain as the page it is included on. Loading it from a local file doesn't seem to work either. So it would only work if we put the first run page on our server.
My comments now are mostly nits. However, determining the iframe content size is
possible if the code inside the iframe cooperates. It should use
window.postMessage() to notify the parent frame, something like:
parent.postMessage({
width: Math.max(document.body.offsetWidth || document.body.scrollWidth),
height: Math.max(document.body.offsetHeight || document.body.scrollHeight)
}, "*");
Note: The parent frame should make sure to check the message origin just in
case.
http://codereview.adblockplus.org/8615139/diff/19002/firstRun.js
File firstRun.js (right):
http://codereview.adblockplus.org/8615139/diff/19002/firstRun.js#newcode19
firstRun.js:19: }
Missing semicolon?
http://codereview.adblockplus.org/8615139/diff/19002/firstRun.js#newcode26
firstRun.js:26: glassPane.className = "visible";
Does it still work if you close the popup and then open it again?
http://codereview.adblockplus.org/8615139/diff/19002/firstRun.js#newcode35
firstRun.js:35: for (var i = 0; i < links.length; i++)
I consider it acceptable to omit brackets if the for loop body only spans one
line - for multiple lines like here it really hurts readability however, even
though syntactically there is no issue.
http://codereview.adblockplus.org/8615139/diff/19002/firstRun.js#newcode40
firstRun.js:40: openSharePopup("http://localhost:8000");
Debug code apparently.
http://codereview.adblockplus.org/8615139/diff/19002/skin/firstRun.css
File skin/firstRun.css (right):
http://codereview.adblockplus.org/8615139/diff/19002/skin/firstRun.css#newcod...
skin/firstRun.css:149: transition-property: opactiy, visibility;
opactiy => opacity?
http://codereview.adblockplus.org/8615139/diff/19002/skin/firstRun.css#newcod...
skin/firstRun.css:155: transition-duration: 0.2s;
Maybe merge this with the #glass-pane rules above?
http://codereview.adblockplus.org/8615139/diff/19002/skin/firstRun.css#newcod...
skin/firstRun.css:162: }
Maybe merge this with the #share-popup rules above?
LGTM but I would still prefer to see the automatic sizing (via postMessage) implemented.
Here are my comments, forgot to publish them. http://codereview.adblockplus.org/8615139/diff/19002/firstRun.js File firstRun.js (right): http://codereview.adblockplus.org/8615139/diff/19002/firstRun.js#newcode19 firstRun.js:19: } On 2012/10/25 10:45:03, Wladimir Palant wrote: > Missing semicolon? Yes, ouch. http://codereview.adblockplus.org/8615139/diff/19002/firstRun.js#newcode26 firstRun.js:26: glassPane.className = "visible"; On 2012/10/25 10:45:03, Wladimir Palant wrote: > Does it still work if you close the popup and then open it again? Sure. http://codereview.adblockplus.org/8615139/diff/19002/firstRun.js#newcode35 firstRun.js:35: for (var i = 0; i < links.length; i++) On 2012/10/25 10:45:03, Wladimir Palant wrote: > I consider it acceptable to omit brackets if the for loop body only spans one > line - for multiple lines like here it really hurts readability however, even > though syntactically there is no issue. OK, seems appropriate. http://codereview.adblockplus.org/8615139/diff/19002/firstRun.js#newcode40 firstRun.js:40: openSharePopup("http://localhost:8000"); On 2012/10/25 10:45:03, Wladimir Palant wrote: > Debug code apparently. Double ouch. http://codereview.adblockplus.org/8615139/diff/19002/skin/firstRun.css File skin/firstRun.css (right): http://codereview.adblockplus.org/8615139/diff/19002/skin/firstRun.css#newcod... skin/firstRun.css:149: transition-property: opactiy, visibility; On 2012/10/25 10:45:03, Wladimir Palant wrote: > opactiy => opacity? Yes, ouch. http://codereview.adblockplus.org/8615139/diff/19002/skin/firstRun.css#newcod... skin/firstRun.css:155: transition-duration: 0.2s; On 2012/10/25 10:45:03, Wladimir Palant wrote: > Maybe merge this with the #glass-pane rules above? This is actually what I came up with fine tuning the animation. I think it's worth keeping, feels a little better. http://codereview.adblockplus.org/8615139/diff/19002/skin/firstRun.css#newcod... skin/firstRun.css:162: } On 2012/10/25 10:45:03, Wladimir Palant wrote: > Maybe merge this with the #share-popup rules above? Same as above.
Uploaded a new changeset, the popup size is no longer hard coded.
LGTM with the one debug statement removed. http://codereview.adblockplus.org/8615139/diff/16004/firstRun.js File firstRun.js (right): http://codereview.adblockplus.org/8615139/diff/16004/firstRun.js#newcode16 firstRun.js:16: console.log(event); Debug code? 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
