Issue 29592569: Issue 5943 - Implement Updates Page for Adblock Plus extension (Closed)
|
Created:
Oct. 30, 2017, 10:37 a.m. by martin Modified:
Jan. 12, 2018, 4:04 p.m. CC:
juliandoucette Base URL:
https://hg.adblockplus.org/adblockplusui/ Visibility:
Public. |
DescriptionIssue 5943 - Implement Updates Page for Adblock Plus extension
Patch Set 1 #
Total comments: 26
Patch Set 2 : Addressed initial round of feedback #
Total comments: 65
Patch Set 3 : Addressed second round of feedback #Patch Set 4 : Addressed second round of feedback #
Total comments: 32
Patch Set 5 : Addressed third round of feedback #
Total comments: 14
Patch Set 6 : Addressed fourth round of feedback #
Total comments: 15
Patch Set 7 : Addressed fifth round of feedback #
Total comments: 21
Patch Set 8 : Addressed sixth round of feedback #
Total comments: 14
Patch Set 9 : Addressed seventh round of feedback #
Total comments: 1
Patch Set 10 : Addressed seventh round of feedback #
MessagesTotal messages: 26
Hello there, Submitted an initial implementation of the updates page. Will try and address any comments as fast as possible.
Is this supposed to work on mobile? https://codereview.adblockplus.org/29592569/diff/29592570/skin/updates.css File skin/updates.css (right): https://codereview.adblockplus.org/29592569/diff/29592570/skin/updates.css#ne... skin/updates.css:11: @font-face NIT: I'm guessing my comment about us using 600 semibold instead of 700 applies here too? https://codereview.adblockplus.org/29592569/diff/29592570/skin/updates.css#ne... skin/updates.css:37: .graphic-column This class and the one below have enough in common that it warrants a single selector e.g. .column { height: 100vh; width: 50%; display: flex; align-items: center; justify-content: center; } #graphic-column { background: #8DC446; } https://codereview.adblockplus.org/29592569/diff/29592570/skin/updates.css#ne... skin/updates.css:57: .content-column-entries NIT: We generally use IDs for things that only occur once. https://codereview.adblockplus.org/29592569/diff/29592570/skin/updates.css#ne... skin/updates.css:63: .content-column-entries a { NIT: .content a would work fine here https://codereview.adblockplus.org/29592569/diff/29592570/updates.html File updates.html (right): https://codereview.adblockplus.org/29592569/diff/29592570/updates.html#newcode3 updates.html:3: - This file is part of Adblock Plus <https://adblockplus.org/>, This file isn't part of adblockplus.org https://codereview.adblockplus.org/29592569/diff/29592570/updates.html#newcode25 updates.html:25: <link type="text/css" href="skin/defaults.css" rel="stylesheet" /> You didn't upload default.css with this review or mention that this review should apply on top of another. https://codereview.adblockplus.org/29592569/diff/29592570/updates.html#newcode36 updates.html:36: <main> <main> has no heading. I suggest that you use the first <section> as a <header> instead to fix this. https://codereview.adblockplus.org/29592569/diff/29592570/updates.html#newcode38 updates.html:38: <div class="container content full-width-container"> .container and .full-width-container are contradictory (.container is for fixed-width containers.) There is a defaults.css class called full-width that could replace both of these here. https://codereview.adblockplus.org/29592569/diff/29592570/updates.html#newcode60 updates.html:60: <h2>What's new?</h2> NIT: It's recommended to use header instead of hgroup and <p> instead of <h2> for this type of component. See http://html5doctor.com/howto-subheadings/ https://codereview.adblockplus.org/29592569/diff/29592570/updates.html#newcode63 updates.html:63: <div class="feature-entry"> Suggest: I think these could be <article>s https://codereview.adblockplus.org/29592569/diff/29592570/updates.html#newcode65 updates.html:65: <p>All new Options page - revamped and restyled - so it’s easier to customize Adblock Plus just like you want it. <a href="#">Check it out for yourself</a>.</p> NIT: I think the first text could be an inline heading (the same applies below) e.g. <article> <img ...> <div> <h2 class="inline">All new Options page</h2> - revamped and ... </div> </article> Suggest: Make these not-inline headings. https://codereview.adblockplus.org/29592569/diff/29592570/updates.html#newcode76 updates.html:76: <img class="feature-icon" src="/skin/updates-page/icon-mobile.svg" alt="thumbs-up-icon"/> NIT: The phone alignment looks weird. Maybe align it vertically center with the heading and text and then put an line above the app store buttons?
(Note: Above are my first impressions. I stopped reviewing after updates.html and the first part of updates.css.)
Hey all. I (hopefully) addressed the first feedback round. @juliandoucette: from what I know from Jeen - this page should not be optimised for tablet / mobile. https://codereview.adblockplus.org/29592569/diff/29592570/skin/updates.css File skin/updates.css (right): https://codereview.adblockplus.org/29592569/diff/29592570/skin/updates.css#ne... skin/updates.css:11: @font-face On 2017/10/30 15:30:10, juliandoucette wrote: > NIT: I'm guessing my comment about us using 600 semibold instead of 700 applies > here too? Changed to 600. https://codereview.adblockplus.org/29592569/diff/29592570/skin/updates.css#ne... skin/updates.css:37: .graphic-column On 2017/10/30 15:30:10, juliandoucette wrote: > This class and the one below have enough in common that it warrants a single > selector e.g. > > .column > { > height: 100vh; > width: 50%; > display: flex; > align-items: center; > justify-content: center; > } > > #graphic-column > { > background: #8DC446; > } Refactored. https://codereview.adblockplus.org/29592569/diff/29592570/skin/updates.css#ne... skin/updates.css:57: .content-column-entries On 2017/10/30 15:30:10, juliandoucette wrote: > NIT: We generally use IDs for things that only occur once. Changed to id="column-content" https://codereview.adblockplus.org/29592569/diff/29592570/updates.html File updates.html (right): https://codereview.adblockplus.org/29592569/diff/29592570/updates.html#newcode3 updates.html:3: - This file is part of Adblock Plus <https://adblockplus.org/>, On 2017/10/30 15:30:11, juliandoucette wrote: > This file isn't part of http://adblockplus.org Hmm it looks like this comment block is in every .html file in the adblockplusui repo. Should I ditch it? https://codereview.adblockplus.org/29592569/diff/29592570/updates.html#newcode25 updates.html:25: <link type="text/css" href="skin/defaults.css" rel="stylesheet" /> On 2017/10/30 15:30:11, juliandoucette wrote: > You didn't upload default.css with this review or mention that this review > should apply on top of another. I actually removed the default.css dependency as I'm not using much of it. https://codereview.adblockplus.org/29592569/diff/29592570/updates.html#newcode36 updates.html:36: <main> On 2017/10/30 15:30:11, juliandoucette wrote: > <main> has no heading. I suggest that you use the first <section> as a <header> > instead to fix this. First <section> is now a <header> element. https://codereview.adblockplus.org/29592569/diff/29592570/updates.html#newcode38 updates.html:38: <div class="container content full-width-container"> On 2017/10/30 15:30:11, juliandoucette wrote: > .container and .full-width-container are contradictory (.container is for > fixed-width containers.) > > There is a defaults.css class called full-width that could replace both of these > here. Ended up simply creating an id called "container". https://codereview.adblockplus.org/29592569/diff/29592570/updates.html#newcode60 updates.html:60: <h2>What's new?</h2> On 2017/10/30 15:30:11, juliandoucette wrote: > NIT: It's recommended to use header instead of hgroup and <p> instead of <h2> > for this type of component. > > See http://html5doctor.com/howto-subheadings/ Oh I've done this so many times, didn't realise it was a sin. Always regarded Heading and Sub-heading as two <h> elements. Anyways, fixed! https://codereview.adblockplus.org/29592569/diff/29592570/updates.html#newcode63 updates.html:63: <div class="feature-entry"> On 2017/10/30 15:30:10, juliandoucette wrote: > Suggest: I think these could be <article>s feature-entries are now <article>s https://codereview.adblockplus.org/29592569/diff/29592570/updates.html#newcode65 updates.html:65: <p>All new Options page - revamped and restyled - so it’s easier to customize Adblock Plus just like you want it. <a href="#">Check it out for yourself</a>.</p> On 2017/10/30 15:30:10, juliandoucette wrote: > NIT: I think the first text could be an inline heading (the same applies below) > e.g. > > <article> > <img ...> > <div> > <h2 class="inline">All new Options page</h2> - revamped and ... > </div> > </article> > > Suggest: Make these not-inline headings. I totally get what you mean, however I don't want to begin doing this. It would only be worth it if we change the whole design so that: each section has a definite title, text block and icon. This would probably require Tom to massage the copy a bit to make it not sound like we've just broken up a bunch of text to smaller chunks. Let's leave this for a later iteration. https://codereview.adblockplus.org/29592569/diff/29592570/updates.html#newcode76 updates.html:76: <img class="feature-icon" src="/skin/updates-page/icon-mobile.svg" alt="thumbs-up-icon"/> On 2017/10/30 15:30:10, juliandoucette wrote: > NIT: The phone alignment looks weird. Maybe align it vertically center with the > heading and text and then put an line above the app store buttons? It doesn't look that bad to me TBH. Every other icon is centred vertically with its corresponding text block. I think it would look even weirder if this last section we align it with the whole block.
Quick reply. Thank Martin! https://codereview.adblockplus.org/29592569/diff/29592570/updates.html File updates.html (right): https://codereview.adblockplus.org/29592569/diff/29592570/updates.html#newcode3 updates.html:3: - This file is part of Adblock Plus <https://adblockplus.org/>, On 2017/11/03 10:02:15, martin wrote: > On 2017/10/30 15:30:11, juliandoucette wrote: > > This file isn't part of http://adblockplus.org > > Hmm it looks like this comment block is in every .html file in the adblockplusui > repo. Should I ditch it? I was mistaken. https://codereview.adblockplus.org/29592569/diff/29592570/updates.html#newcode65 updates.html:65: <p>All new Options page - revamped and restyled - so it’s easier to customize Adblock Plus just like you want it. <a href="#">Check it out for yourself</a>.</p> On 2017/11/03 10:02:14, martin wrote: > I totally get what you mean, however I don't want to begin doing this. It would > only be worth it if we change the whole design so that: each section has a > definite title, text block and icon. This would probably require Tom to massage > the copy a bit to make it not sound like we've just broken up a bunch of text to > smaller chunks. Let's leave this for a later iteration. Ack. I think that these should only be <article>s if they have headings (inline or screen reader only). https://codereview.adblockplus.org/29592569/diff/29592570/updates.html#newcode76 updates.html:76: <img class="feature-icon" src="/skin/updates-page/icon-mobile.svg" alt="thumbs-up-icon"/> On 2017/11/03 10:02:13, martin wrote: > On 2017/10/30 15:30:10, juliandoucette wrote: > > NIT: The phone alignment looks weird. Maybe align it vertically center with > the > > heading and text and then put an line above the app store buttons? > > It doesn't look that bad to me TBH. Every other icon is centred vertically with > its corresponding text block. I think it would look even weirder if this last > section we align it with the whole block. Acknowledged. Looks fine to me on second glance :/
The main points I mentioned are around translations, links and the app store badges. Apart from that there are only small things I've found. https://codereview.adblockplus.org/29592569/diff/29596582/skin/updates-page/a... File skin/updates-page/abp-logo.svg (right): https://codereview.adblockplus.org/29592569/diff/29596582/skin/updates-page/a... skin/updates-page/abp-logo.svg:1: <svg xmlns="http://www.w3.org/2000/svg" viewBox="6300.545 4043.125 162.032 162.032"><defs><style>.a{fill:#fff;}.b{fill:#c70d2c;}.c{fill:#d2d2d2;}</style></defs><path class="a" d="M6438.852 4066.85l-23.735-23.725h-67.111l-47.461 47.461v67.121l23.725 23.725 23.735 23.725h67.121l47.451-47.461v-67.11zm13.031 86.417l-41.2 41.2h-58.248l-20.623-20.626-20.622-20.622v-58.204l41.245-41.2h58.253l20.622 20.622 20.622 20.622zm-36.766-110.142h-67.111l-47.461 47.461v67.121l47.461 47.451h67.121l47.451-47.461v-67.111z"/><path class="b" d="M6352.434 4194.463l-41.2-41.2v-58.249l41.2-41.2h58.253l41.2 41.2v58.253l-41.2 41.2z"/><path class="a" d="M6345.138 4138.872h-14.73l-2.946 13.173h-11.038l15.054-55.4h13.061l15.054 55.386h-11.509zm-2.21-9.771l-1.139-5.1q-1.035-4.156-1.964-8.835t-1.964-9.005h-.295q-.884 4.419-1.807 9.044t-1.963 8.84l-1.178 5.1z"/><path class="a" d="M67 54.5h16.439a35.863 35.863 0 0 1 6.972.638 15.271 15.271 0 0 1 5.607 2.249 11.313 11.313 0 0 1 3.761 4.252 14.328 14.328 0 0 1 1.365 6.629 15 15 0 0 1-.481 3.741 13.228 13.228 0 0 1-1.443 3.491 11.027 11.027 0 0 1-2.514 2.887 9.947 9.947 0 0 1-3.614 1.866v.344a13.4 13.4 0 0 1 7.787 4.252q2.622 3.142 2.622 8.75a17.018 17.018 0 0 1-1.444 7.306 13.748 13.748 0 0 1-3.987 5.057 17.224 17.224 0 0 1-5.94 2.938 26.3 26.3 0 0 1-7.306.982h-17.824zm15.918 22.341q3.987 0 5.794-1.827a6.756 6.756 0 0 0 1.856-4.969q0-3.142-1.846-4.507t-5.686-1.355h-5.236v12.658zm1.041 23.323q8.926 0 8.926-7.562 0-3.653-2.21-5.313t-6.717-1.66h-6.158v14.534z" transform="translate(6299.34 4042.145)"/><path class="a" d="M111.78 54.5h17.568a29.107 29.107 0 0 1 7.345.894 15.457 15.457 0 0 1 5.98 2.946 14.219 14.219 0 0 1 4.026 5.519 21.447 21.447 0 0 1 1.473 8.455 21.447 21.447 0 0 1-1.512 8.406 16.105 16.105 0 0 1-4.1 5.892 16.7 16.7 0 0 1-6.02 3.4 23.225 23.225 0 0 1-7.237 1.1h-6.717v18.776h-10.8zm16.91 26.681q8.838 0 8.838-8.838 0-4.331-2.249-6.118t-6.589-1.787h-6.049v16.694z" transform="translate(6298.534 4042.145)"/><path class="c" d="M6415.117 4043.125h-67.111l-47.461 47.461v67.121l47.461 47.451h67.121l47.451-47.461v-67.111zm44.191 113.236l-45.526 45.556h-64.437l-45.559-45.556v-64.436l45.559-45.559h64.43l45.562 45.559z"/></svg> Can't we just use the existing Adblock Plus logo SVG file? The only difference appears to be that this one has a slightly thicker gray border. https://codereview.adblockplus.org/29592569/diff/29596582/skin/updates-page/g... File skin/updates-page/googleplay-bg.svg (right): https://codereview.adblockplus.org/29592569/diff/29596582/skin/updates-page/g... skin/updates-page/googleplay-bg.svg:1: <svg width="180" height="54" viewBox="0 0 180 54" xmlns="http://www.w3.org/2000/svg"><title>googleplay-bg</title><desc>Created with Sketch.</desc><g fill="none"><rect fill="#000" width="179.719" height="54" rx="8.438"/><path d="M59.202 18.75v-8.998h2.54c.783 0 1.475.173 2.076.519.602.346 1.066.838 1.394 1.477.328.639.493 1.372.497 2.2v.575c0 .849-.164 1.592-.491 2.231-.328.639-.795 1.129-1.403 1.471-.608.342-1.315.517-2.123.525h-2.49zm1.187-8.021v7.051h1.248c.915 0 1.626-.284 2.135-.853.509-.569.763-1.378.763-2.429v-.525c0-1.022-.24-1.816-.72-2.382-.48-.566-1.161-.854-2.042-.862h-1.384zm7.643 4.616c0-.655.129-1.244.386-1.767.257-.523.616-.927 1.075-1.211.459-.284.984-.426 1.573-.426.91 0 1.647.315 2.209.946.562.63.844 1.469.844 2.515v.08c0 .651-.125 1.235-.374 1.752-.249.517-.606.92-1.069 1.208-.463.288-.996.433-1.597.433-.906 0-1.641-.315-2.203-.946-.562-.63-.844-1.465-.844-2.503v-.08zm1.149.136c0 .742.172 1.337.516 1.786.344.449.804.674 1.381.674.581 0 1.042-.228 1.384-.683.342-.455.513-1.093.513-1.913 0-.733-.174-1.328-.522-1.783-.348-.455-.811-.683-1.387-.683-.564 0-1.02.225-1.366.674-.346.449-.519 1.092-.519 1.928zm13.304 1.693l1.285-5.111h1.143l-1.947 6.687h-.927l-1.625-5.067-1.582 5.067h-.927l-1.94-6.687h1.137l1.316 5.006 1.557-5.006h.921l1.588 5.111zm5.684-5.111l.037.84c.511-.643 1.178-.964 2.002-.964 1.413 0 2.126.797 2.138 2.392v4.419h-1.143v-4.425c-.004-.482-.114-.838-.331-1.069-.216-.231-.553-.346-1.01-.346-.371 0-.696.099-.976.297-.28.198-.499.457-.655.779v4.765h-1.143v-6.687h1.081zm8.156 6.687h-1.143v-9.492h1.143v9.492zm2.539-3.405c0-.655.129-1.244.386-1.767.257-.523.616-.927 1.075-1.211.459-.284.984-.426 1.573-.426.91 0 1.647.315 2.209.946.562.63.844 1.469.844 2.515v.08c0 .651-.125 1.235-.374 1.752-.249.517-.606.92-1.069 1.208-.463.288-.996.433-1.597.433-.906 0-1.641-.315-2.203-.946-.562-.63-.844-1.465-.844-2.503v-.08zm1.149.136c0 .742.172 1.337.516 1.786.344.449.804.674 1.381.674.581 0 1.042-.228 1.384-.683.342-.455.513-1.093.513-1.913 0-.733-.174-1.328-.522-1.783-.348-.455-.811-.683-1.387-.683-.564 0-1.02.225-1.366.674-.346.449-.519 1.092-.519 1.928zm11.512 3.269c-.066-.132-.119-.367-.161-.704-.531.552-1.166.828-1.903.828-.659 0-1.2-.186-1.622-.559-.422-.373-.633-.846-.633-1.418 0-.696.265-1.237.794-1.622s1.274-.578 2.234-.578h1.112v-.525c0-.4-.119-.718-.358-.955s-.591-.355-1.057-.355c-.408 0-.75.103-1.026.309-.276.206-.414.455-.414.748h-1.149c0-.334.118-.656.355-.967.237-.311.558-.557.964-.738.406-.181.852-.272 1.338-.272.77 0 1.374.193 1.811.578.437.385.663.916.68 1.591v3.078c0 .614.078 1.102.235 1.465v.099h-1.199zm-1.897-.871c.358 0 .698-.093 1.02-.278.321-.185.554-.426.698-.723v-1.372h-.896c-1.401 0-2.101.41-2.101 1.23 0 .358.119.639.358.84.239.202.546.303.921.303zm5.388-2.528c0-1.026.243-1.851.729-2.475.486-.624 1.123-.936 1.91-.936.783 0 1.403.268 1.86.803v-3.485h1.143v9.492h-1.051l-.056-.717c-.457.56-1.094.84-1.91.84-.775 0-1.406-.317-1.894-.952-.488-.634-.732-1.463-.732-2.484v-.087zm1.143.13c0 .758.157 1.351.47 1.78.313.428.746.643 1.298.643.725 0 1.255-.325 1.588-.976v-3.071c-.342-.63-.867-.946-1.576-.946-.56 0-.997.216-1.31.649-.313.433-.47 1.073-.47 1.922zm11.128-.136c0-.655.129-1.244.386-1.767.257-.523.616-.927 1.075-1.211.459-.284.984-.426 1.573-.426.91 0 1.647.315 2.209.946.562.63.844 1.469.844 2.515v.08c0 .651-.125 1.235-.374 1.752-.249.517-.606.92-1.069 1.208-.463.288-.996.433-1.597.433-.906 0-1.641-.315-2.203-.946-.562-.63-.844-1.465-.844-2.503v-.08zm1.149.136c0 .742.172 1.337.516 1.786.344.449.804.674 1.381.674.581 0 1.042-.228 1.384-.683.342-.455.513-1.093.513-1.913 0-.733-.174-1.328-.522-1.783-.348-.455-.811-.683-1.387-.683-.564 0-1.02.225-1.366.674-.346.449-.519 1.092-.519 1.928zm8.465-3.417l.037.84c.511-.643 1.178-.964 2.002-.964 1.413 0 2.126.797 2.138 2.392v4.419h-1.143v-4.425c-.004-.482-.114-.838-.331-1.069-.216-.231-.553-.346-1.01-.346-.371 0-.696.099-.976.297-.28.198-.499.457-.655.779v4.765h-1.143v-6.687h1.081zM133.332 37.385c0 1.668-.369 2.991-1.107 3.97-.738.979-1.727 1.468-2.966 1.468-1.463 0-2.584-.514-3.362-1.542v5.458h-1.177v-14.812h1.098l.059 1.513c.771-1.14 1.889-1.711 3.352-1.711 1.279 0 2.282.484 3.011 1.453.728.969 1.093 2.314 1.093 4.034v.168zm-1.187-.208c0-1.365-.28-2.442-.84-3.233-.56-.791-1.341-1.187-2.343-1.187-.725 0-1.348.175-1.869.524-.521.349-.92.857-1.196 1.523v5.132c.283.613.689 1.081 1.216 1.404.527.323 1.15.484 1.869.484.995 0 1.772-.397 2.329-1.191.557-.794.836-1.946.836-3.456zm5.132 5.448h-1.187v-15.188h1.187v15.188zm9.967 0c-.119-.336-.194-.834-.227-1.493-.415.541-.944.957-1.587 1.251-.643.293-1.323.44-2.042.44-1.028 0-1.861-.287-2.497-.86-.636-.573-.954-1.299-.954-2.175 0-1.042.433-1.865 1.3-2.472.867-.606 2.075-.91 3.624-.91h2.146v-1.216c0-.765-.236-1.366-.707-1.805-.471-.438-1.159-.658-2.062-.658-.824 0-1.506.211-2.047.633-.541.422-.811.929-.811 1.523l-1.187-.01c0-.85.396-1.587 1.187-2.21s1.763-.934 2.917-.934c1.193 0 2.134.298 2.823.895.689.597 1.043 1.429 1.063 2.497v5.063c0 1.035.109 1.809.326 2.324v.119h-1.266zm-3.718-.85c.791 0 1.498-.191 2.121-.573.623-.382 1.076-.893 1.36-1.533v-2.353h-2.116c-1.18.013-2.103.229-2.769.648-.666.419-.999.994-.999 1.725 0 .6.222 1.098.667 1.493.445.396 1.023.593 1.735.593zm11.203-.85l3.125-8.998h1.276l-4.568 12.518-.237.554c-.587 1.299-1.493 1.948-2.719 1.948-.283 0-.587-.046-.91-.138l-.01-.979.613.059c.58 0 1.05-.143 1.409-.43.359-.287.664-.779.915-1.478l.524-1.444-4.034-10.609h1.295l3.322 8.998zM83.722 37.484c0 2.838-2.22 4.929-4.944 4.929-2.724 0-4.944-2.091-4.944-4.929 0-2.858 2.22-4.929 4.944-4.929 2.724 0 4.944 2.071 4.944 4.929zm-2.164 0c0-1.773-1.287-2.987-2.78-2.987-1.493 0-2.78 1.213-2.78 2.987 0 1.756 1.287 2.987 2.78 2.987 1.493 0 2.78-1.233 2.78-2.987zm12.831 0c0 2.838-2.22 4.929-4.944 4.929-2.724 0-4.944-2.091-4.944-4.929 0-2.856 2.22-4.929 4.944-4.929 2.724 0 4.944 2.071 4.944 4.929zm-2.164 0c0-1.773-1.287-2.987-2.78-2.987-1.493 0-2.78 1.213-2.78 2.987 0 1.756 1.287 2.987 2.78 2.987 1.493 0 2.78-1.233 2.78-2.987zm12.386-4.631v8.849c0 3.64-2.147 5.127-4.684 5.127-2.389 0-3.827-1.598-4.369-2.904l1.884-.784c.336.802 1.158 1.749 2.482 1.749 1.624 0 2.631-1.002 2.631-2.889v-.709h-.076c-.484.598-1.418 1.12-2.596 1.12-2.464 0-4.722-2.147-4.722-4.909 0-2.782 2.258-4.947 4.722-4.947 1.176 0 2.109.522 2.596 1.102h.076v-.802h2.056v-.002zm-1.902 4.649c0-1.736-1.158-3.004-2.631-3.004-1.493 0-2.744 1.269-2.744 3.004 0 1.718 1.251 2.969 2.744 2.969 1.473 0 2.631-1.251 2.631-2.969zm5.291-9.835v14.444h-2.111v-14.444zm8.227 11.44l1.68 1.12c-.542.802-1.849 2.184-4.107 2.184-2.8 0-4.891-2.164-4.891-4.929 0-2.931 2.109-4.929 4.649-4.929 2.558 0 3.809 2.036 4.218 3.136l.224.56-6.589 2.729c.504.989 1.289 1.493 2.389 1.493 1.102 0 1.867-.542 2.427-1.364zm-5.171-1.773l4.404-1.829c-.242-.616-.971-1.044-1.829-1.044-1.1 0-2.631.971-2.576 2.873zm-45.214-1.132v-2.091h7.047c.069.364.104.796.104 1.262 0 1.569-.429 3.509-1.811 4.891-1.344 1.4-3.062 2.147-5.338 2.147-4.218 0-7.764-3.436-7.764-7.653 0-4.218 3.547-7.653 7.764-7.653 2.333 0 3.996.916 5.244 2.109l-1.476 1.476c-.896-.84-2.109-1.493-3.771-1.493-3.08 0-5.489 2.482-5.489 5.562s2.409 5.562 5.489 5.562c1.998 0 3.136-.802 3.864-1.531.591-.591.98-1.436 1.133-2.589l-4.998.002zM13.5 41.566v-29.129c0-1.016.589-1.895 1.444-2.312l16.85 16.872-16.857 16.878c-.852-.418-1.438-1.295-1.438-2.309zm23.64-9.217l-18.424 10.651 14.531-14.549 3.893 3.899zm5.727-7.39c.613.47 1.008 1.21 1.008 2.042 0 .82-.383 1.55-.98 2.021l-3.913 2.262-4.283-4.288 4.28-4.285 3.889 2.248zm-24.14-13.955l18.41 10.643-3.891 3.896-14.52-14.539z" fill="#fff"/></g></svg> Detail: I noticed that this SVG still contains information that has been removed from all other files (i.e. `<title>googleplay-bg</title><desc>Created with Sketch.</desc>`). https://codereview.adblockplus.org/29592569/diff/29596582/skin/updates.css File skin/updates.css (right): https://codereview.adblockplus.org/29592569/diff/29596582/skin/updates.css#ne... skin/updates.css:3: font-family: "Source Sans Pro"; Coding style: "Indentation: Two spaces per logic level" See https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style... https://codereview.adblockplus.org/29592569/diff/29596582/skin/updates.css#ne... skin/updates.css:39: @media(max-width: 960px) I'd be curious to hear what Julian's opinion is on this because usually we place all conditional styles at the bottom to have a better overview of the differences for certain screen sizes. https://codereview.adblockplus.org/29592569/diff/29596582/skin/updates.css#ne... skin/updates.css:58: background: #8DC446; Detail: I'd recommend avoiding shorthand properties if they don't add any value because it not only overrides "background-color" but also all other "background-*" properties that have previously been defined. Also applies to other occurrences in this file. https://codereview.adblockplus.org/29592569/diff/29596582/skin/updates.css#ne... skin/updates.css:84: margin: 0 0 2em 5em; Detail: This may produce unexpected results for right-to-left languages. https://codereview.adblockplus.org/29592569/diff/29596582/skin/updates.css#ne... skin/updates.css:89: opacity: .5; Coding style: "Don't omit the optional leading 0 for decimal numbers." See https://adblockplus.org/en/coding-style#html-css https://codereview.adblockplus.org/29592569/diff/29596582/skin/updates.css#ne... skin/updates.css:90: font-weight: normal; Detail: For consistency, we use numeric values for the font weight. https://codereview.adblockplus.org/29592569/diff/29596582/skin/updates.css#ne... skin/updates.css:112: background: url('/skin/updates-page/base-graphic.svg'); Coding style: "Do not use quotation marks in URI values (url())." See https://google.github.io/styleguide/htmlcssguide.html#CSS_Quotation_Marks https://codereview.adblockplus.org/29592569/diff/29596582/skin/updates.css#ne... skin/updates.css:112: background: url('/skin/updates-page/base-graphic.svg'); Suggestion: Mind using the same name for the directory as for the update page itself (i.e. "updates" instead of "updates-page") for consistency? Otherwise this may bite us later on. https://codereview.adblockplus.org/29592569/diff/29596582/skin/updates.css#ne... skin/updates.css:112: background: url('/skin/updates-page/base-graphic.svg'); Detail: Note that we've been using relative paths on other pages so I'd recommend changing the paths throughout this file for consistency in case the absolute path changes in the future. https://codereview.adblockplus.org/29592569/diff/29596582/skin/updates.css#ne... skin/updates.css:131: left: 100px; Detail: This may produce unexpected results for right-to-left languages. https://codereview.adblockplus.org/29592569/diff/29596582/skin/updates.css#ne... skin/updates.css:163: margin: 0 1em 0 0; Detail: This may produce unexpected results for right-to-left languages. https://codereview.adblockplus.org/29592569/diff/29596582/skin/updates.css#ne... skin/updates.css:169: hegiht: 58px; Typo: Replace "hegiht" with "height" https://codereview.adblockplus.org/29592569/diff/29596582/skin/updates.css#ne... skin/updates.css:170: margin-left: 5em; Detail: This may produce unexpected results for right-to-left languages. https://codereview.adblockplus.org/29592569/diff/29596582/skin/updates.css#ne... skin/updates.css:180: float: left; Detail: This may produce unexpected results for right-to-left languages. In general, I'd recommend avoiding floats as much as possible since they can lead to all sorts of trouble. https://codereview.adblockplus.org/29592569/diff/29596582/updates.html File updates.html (right): https://codereview.adblockplus.org/29592569/diff/29596582/updates.html#newcode20 updates.html:20: <title class="i18n_firstRun_title"></title> Coding style: "Indentation: Two spaces per logic level" See https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style... https://codereview.adblockplus.org/29592569/diff/29596582/updates.html#newcode20 updates.html:20: <title class="i18n_firstRun_title"></title> This class name refers to the wrong message ID. https://codereview.adblockplus.org/29592569/diff/29596582/updates.html#newcode22 updates.html:22: <meta name="viewport" content="width=device-width, initial-scale=1" /> I'm fine with adding this to have a more pleasant experience on mobile devices although the design is a bit broken because the text "Adblock Plus has been updated!" is oddly placed on smaller screens. Just wanted to mention this in case you haven't been aware of this breakage in the design. https://codereview.adblockplus.org/29592569/diff/29596582/updates.html#newcode23 updates.html:23: <link type="text/css" href="skin/common.css" rel="stylesheet" /> Detail: None of the styles included in common.css are used for this page so let's not include it. https://codereview.adblockplus.org/29592569/diff/29596582/updates.html#newcode25 updates.html:25: <script type="text/javascript" src="polyfill.js"></script> Detail: The "type" attribute has been made optional in HTML5 so we don't need to specify it anymore. https://codereview.adblockplus.org/29592569/diff/29596582/updates.html#newcode40 updates.html:40: <img src="/skin/updates-page/abp-logo.svg" alt="adblock-plus-logo"/> Detail: Texts in "alt" attributes are just as visible to visually-impaired users as other texts which is why we should translate them. However, in this case it merely describes a decorative element so I'd suggest removing it here and for all other occurrences where this applies. https://codereview.adblockplus.org/29592569/diff/29596582/updates.html#newcode42 updates.html:42: <h2>Update Complete</h2> Detail: I noticed that if this text is longer (i.e. when it's translated to a different language) that it can slightly overflow to the right so we could simply add a margin to `.version-details` to avoid this issue. https://codereview.adblockplus.org/29592569/diff/29596582/updates.html#newcode42 updates.html:42: <h2>Update Complete</h2> We mentioned it in today's meeting but just for completeness sake I wanted to note that we want to have the text on this page be translatable. https://codereview.adblockplus.org/29592569/diff/29596582/updates.html#newcode43 updates.html:43: <span>v1.13.4</span> Detail: The version number may end up being different than what we define here so I think we got two options: Either we remove the version number or we set it dynamically the way we do it in the new options page (see https://hg.adblockplus.org/adblockplusui/file/tip/desktop-options.js#l878). https://codereview.adblockplus.org/29592569/diff/29596582/updates.html#newcode60 updates.html:60: <p>All new Options page - revamped and restyled - so it’s easier to customize Adblock Plus just like you want it. <a href="#">Check it out for yourself</a>.</p> Detail: We try to normalize strings, for instance by replacing various variants of apostrophe's such as "’" with "'", to avoid running into issues with translations, screen readers, etc. https://codereview.adblockplus.org/29592569/diff/29596582/updates.html#newcode60 updates.html:60: <p>All new Options page - revamped and restyled - so it’s easier to customize Adblock Plus just like you want it. <a href="#">Check it out for yourself</a>.</p> This link and the app store buttons below are missing the URL. This one I don't see in the spec either. Note that we use documentation links (see https://bitbucket.org/adblockplus/spec/src/8e6b8ee2b7a1484344688baa405e11ab42...) so in case those links are not yet specified in the spec, we should add them and create the necessary redirect for them on our servers. The reason for that is that links may break over time and due to the fact that people can install this version of the extension at any point in time in the future. Therefore we link to adblockplus.org instead from where we redirect the user to the appropriate URL. https://codereview.adblockplus.org/29592569/diff/29596582/updates.html#newcode65 updates.html:65: <p>Block ads on Facebook again! New blocking tech allows for fast fixes in case <strike>the Empire</strike> Facebook strikes back.</p> The `<strike>` element has been deprecated in HTML4 according to https://developer.mozilla.org/en-US/docs/Web/HTML/Element/strike. Instead, it suggests to use either `<del>` or `<s>` but we only support `<strong>` and `<a>` in translatable strings. Therefore I'd recommend using `<strong>` and styling it via CSS. https://codereview.adblockplus.org/29592569/diff/29596582/updates.html#newcode79 updates.html:79: <img src="skin/updates-page/googleplay-bg.svg" alt="google play store button"> Are we allowed to use those images? Because according to https://play.google.com/intl/en_us/badges/ we're not allowed to alter the badges and presumably, Apple's guidelines have similar requirements. https://codereview.adblockplus.org/29592569/diff/29596582/updates.html#newcode79 updates.html:79: <img src="skin/updates-page/googleplay-bg.svg" alt="google play store button"> These images contain text which we means that we cannot translate it. Therefore we need to specify the text in the HTML and either style the button using HTML and CSS or make changes to the design.
Hey all, I submitted a second review, addressing Thomas's feedback. Let me know if I missed something. https://codereview.adblockplus.org/29592569/diff/29596582/skin/updates.css File skin/updates.css (right): https://codereview.adblockplus.org/29592569/diff/29596582/skin/updates.css#ne... skin/updates.css:3: font-family: "Source Sans Pro"; On 2017/11/03 17:40:01, Thomas Greiner wrote: > Coding style: "Indentation: Two spaces per logic level" > > See > https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style... Done. https://codereview.adblockplus.org/29592569/diff/29596582/skin/updates.css#ne... skin/updates.css:39: @media(max-width: 960px) On 2017/11/03 17:40:00, Thomas Greiner wrote: > I'd be curious to hear what Julian's opinion is on this because usually we place > all conditional styles at the bottom to have a better overview of the > differences for certain screen sizes. Moved the query to the bottom of the file. https://codereview.adblockplus.org/29592569/diff/29596582/skin/updates.css#ne... skin/updates.css:58: background: #8DC446; On 2017/11/03 17:40:00, Thomas Greiner wrote: > Detail: I'd recommend avoiding shorthand properties if they don't add any value > because it not only overrides "background-color" but also all other > "background-*" properties that have previously been defined. > > Also applies to other occurrences in this file. Done. https://codereview.adblockplus.org/29592569/diff/29596582/skin/updates.css#ne... skin/updates.css:84: margin: 0 0 2em 5em; On 2017/11/03 17:40:00, Thomas Greiner wrote: > Detail: This may produce unexpected results for right-to-left languages. Done. https://codereview.adblockplus.org/29592569/diff/29596582/skin/updates.css#ne... skin/updates.css:89: opacity: .5; On 2017/11/03 17:40:01, Thomas Greiner wrote: > Coding style: "Don't omit the optional leading 0 for decimal numbers." > > See https://adblockplus.org/en/coding-style#html-css Done. https://codereview.adblockplus.org/29592569/diff/29596582/skin/updates.css#ne... skin/updates.css:89: opacity: .5; On 2017/11/03 17:40:01, Thomas Greiner wrote: > Coding style: "Don't omit the optional leading 0 for decimal numbers." > > See https://adblockplus.org/en/coding-style#html-css Done. https://codereview.adblockplus.org/29592569/diff/29596582/skin/updates.css#ne... skin/updates.css:90: font-weight: normal; On 2017/11/03 17:40:00, Thomas Greiner wrote: > Detail: For consistency, we use numeric values for the font weight. Done. https://codereview.adblockplus.org/29592569/diff/29596582/skin/updates.css#ne... skin/updates.css:112: background: url('/skin/updates-page/base-graphic.svg'); On 2017/11/03 17:40:00, Thomas Greiner wrote: > Suggestion: Mind using the same name for the directory as for the update page > itself (i.e. "updates" instead of "updates-page") for consistency? Otherwise > this may bite us later on. Done. https://codereview.adblockplus.org/29592569/diff/29596582/skin/updates.css#ne... skin/updates.css:112: background: url('/skin/updates-page/base-graphic.svg'); On 2017/11/03 17:40:01, Thomas Greiner wrote: > Coding style: "Do not use quotation marks in URI values (url())." > > See https://google.github.io/styleguide/htmlcssguide.html#CSS_Quotation_Marks Done. https://codereview.adblockplus.org/29592569/diff/29596582/skin/updates.css#ne... skin/updates.css:112: background: url('/skin/updates-page/base-graphic.svg'); On 2017/11/03 17:40:00, Thomas Greiner wrote: > Detail: Note that we've been using relative paths on other pages so I'd > recommend changing the paths throughout this file for consistency in case the > absolute path changes in the future. Done. https://codereview.adblockplus.org/29592569/diff/29596582/skin/updates.css#ne... skin/updates.css:131: left: 100px; On 2017/11/03 17:40:00, Thomas Greiner wrote: > Detail: This may produce unexpected results for right-to-left languages. Done. https://codereview.adblockplus.org/29592569/diff/29596582/skin/updates.css#ne... skin/updates.css:169: hegiht: 58px; On 2017/11/03 17:40:01, Thomas Greiner wrote: > Typo: Replace "hegiht" with "height" Done. https://codereview.adblockplus.org/29592569/diff/29596582/skin/updates.css#ne... skin/updates.css:170: margin-left: 5em; On 2017/11/03 17:40:00, Thomas Greiner wrote: > Detail: This may produce unexpected results for right-to-left languages. Done. https://codereview.adblockplus.org/29592569/diff/29596582/skin/updates.css#ne... skin/updates.css:180: float: left; On 2017/11/03 17:40:01, Thomas Greiner wrote: > Detail: This may produce unexpected results for right-to-left languages. > > In general, I'd recommend avoiding floats as much as possible since they can > lead to all sorts of trouble. Done. https://codereview.adblockplus.org/29592569/diff/29596582/updates.html File updates.html (right): https://codereview.adblockplus.org/29592569/diff/29596582/updates.html#newcode20 updates.html:20: <title class="i18n_firstRun_title"></title> On 2017/11/03 17:40:01, Thomas Greiner wrote: > Coding style: "Indentation: Two spaces per logic level" > > See > https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style... Done. https://codereview.adblockplus.org/29592569/diff/29596582/updates.html#newcode20 updates.html:20: <title class="i18n_firstRun_title"></title> On 2017/11/03 17:40:01, Thomas Greiner wrote: > Coding style: "Indentation: Two spaces per logic level" > > See > https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style... Done. https://codereview.adblockplus.org/29592569/diff/29596582/updates.html#newcode22 updates.html:22: <meta name="viewport" content="width=device-width, initial-scale=1" /> On 2017/11/03 17:40:02, Thomas Greiner wrote: > I'm fine with adding this to have a more pleasant experience on mobile devices > although the design is a bit broken because the text "Adblock Plus has been > updated!" is oddly placed on smaller screens. Just wanted to mention this in > case you haven't been aware of this breakage in the design. This page would never be viewed from a mobile device, so I don't think optimising for mobile should be a priority, however I included a couple of CSS rules to fix the breakage anyways. https://codereview.adblockplus.org/29592569/diff/29596582/updates.html#newcode23 updates.html:23: <link type="text/css" href="skin/common.css" rel="stylesheet" /> On 2017/11/03 17:40:02, Thomas Greiner wrote: > Detail: None of the styles included in common.css are used for this page so > let's not include it. Done. https://codereview.adblockplus.org/29592569/diff/29596582/updates.html#newcode25 updates.html:25: <script type="text/javascript" src="polyfill.js"></script> On 2017/11/03 17:40:02, Thomas Greiner wrote: > Detail: The "type" attribute has been made optional in HTML5 so we don't need to > specify it anymore. Done. https://codereview.adblockplus.org/29592569/diff/29596582/updates.html#newcode40 updates.html:40: <img src="/skin/updates-page/abp-logo.svg" alt="adblock-plus-logo"/> On 2017/11/03 17:40:02, Thomas Greiner wrote: > Detail: Texts in "alt" attributes are just as visible to visually-impaired users > as other texts which is why we should translate them. However, in this case it > merely describes a decorative element so I'd suggest removing it here and for > all other occurrences where this applies. Done. https://codereview.adblockplus.org/29592569/diff/29596582/updates.html#newcode42 updates.html:42: <h2>Update Complete</h2> On 2017/11/03 17:40:02, Thomas Greiner wrote: > We mentioned it in today's meeting but just for completeness sake I wanted to > note that we want to have the text on this page be translatable. Done. https://codereview.adblockplus.org/29592569/diff/29596582/updates.html#newcode42 updates.html:42: <h2>Update Complete</h2> On 2017/11/03 17:40:03, Thomas Greiner wrote: > Detail: I noticed that if this text is longer (i.e. when it's translated to a > different language) that it can slightly overflow to the right so we could > simply add a margin to `.version-details` to avoid this issue. Done. https://codereview.adblockplus.org/29592569/diff/29596582/updates.html#newcode43 updates.html:43: <span>v1.13.4</span> On 2017/11/03 17:40:01, Thomas Greiner wrote: > Detail: The version number may end up being different than what we define here > so I think we got two options: Either we remove the version number or we set it > dynamically the way we do it in the new options page (see > https://hg.adblockplus.org/adblockplusui/file/tip/desktop-options.js#l878). I removed the version number <span> to not delay implementation unnecessarily. https://codereview.adblockplus.org/29592569/diff/29596582/updates.html#newcode60 updates.html:60: <p>All new Options page - revamped and restyled - so it’s easier to customize Adblock Plus just like you want it. <a href="#">Check it out for yourself</a>.</p> On 2017/11/03 17:40:01, Thomas Greiner wrote: > Detail: We try to normalize strings, for instance by replacing various variants > of apostrophe's such as "’" with "'", to avoid running into issues with > translations, screen readers, etc. Done. https://codereview.adblockplus.org/29592569/diff/29596582/updates.html#newcode65 updates.html:65: <p>Block ads on Facebook again! New blocking tech allows for fast fixes in case <strike>the Empire</strike> Facebook strikes back.</p> On 2017/11/03 17:40:02, Thomas Greiner wrote: > The `<strike>` element has been deprecated in HTML4 according to > https://developer.mozilla.org/en-US/docs/Web/HTML/Element/strike. Instead, it > suggests to use either `<del>` or `<s>` but we only support `<strong>` and `<a>` > in translatable strings. > > Therefore I'd recommend using `<strong>` and styling it via CSS. Done. https://codereview.adblockplus.org/29592569/diff/29596582/updates.html#newcode79 updates.html:79: <img src="skin/updates-page/googleplay-bg.svg" alt="google play store button"> On 2017/11/03 17:40:01, Thomas Greiner wrote: > Are we allowed to use those images? Because according to > https://play.google.com/intl/en_us/badges/ we're not allowed to alter the badges > and presumably, Apple's guidelines have similar requirements. Apple store button is in accordance with their guidelines. I exchanged the monochrome graphic with a coloured one for the Google Play button and changed the text. It now adheres to the guidelines. https://codereview.adblockplus.org/29592569/diff/29596582/updates.html#newcode79 updates.html:79: <img src="skin/updates-page/googleplay-bg.svg" alt="google play store button"> On 2017/11/03 17:40:02, Thomas Greiner wrote: > These images contain text which we means that we cannot translate it. Therefore > we need to specify the text in the HTML and either style the button using HTML > and CSS or make changes to the design. We could potentially do that, but it'll slow us down. I truly believe the buttons speak for themselves and there's no need translating the text inside. Not to mention that in order to adhere to the guidelines 100% the fonts inside the buttons must be SanFrancisco and Roboto respectively.
Make sure you're always on the latest revision and avoid reverting changes made by others because I see a couple of those in here. It also makes it difficult to review so it's a good practice to merge your changes after updating to the latest revision and upload that as a patch and then to create a separate patch for the changes to the review itself. Thereby we can differentiate between changes that were necessary because the code base changed and changes that were made to address review comments. Be careful, however, with removing existing patches because that'll also get rid of any review comments they might have. Feel free to remove this patch though and try again because in that case I won't add any further comments to this patch so that none of them gets lost.
Mostly details to address but we should get the links and store buttons resolved. https://codereview.adblockplus.org/29592569/diff/29596582/skin/updates-page/a... File skin/updates-page/abp-logo.svg (right): https://codereview.adblockplus.org/29592569/diff/29596582/skin/updates-page/a... skin/updates-page/abp-logo.svg:1: <svg xmlns="http://www.w3.org/2000/svg" viewBox="6300.545 4043.125 162.032 162.032"><defs><style>.a{fill:#fff;}.b{fill:#c70d2c;}.c{fill:#d2d2d2;}</style></defs><path class="a" d="M6438.852 4066.85l-23.735-23.725h-67.111l-47.461 47.461v67.121l23.725 23.725 23.735 23.725h67.121l47.451-47.461v-67.11zm13.031 86.417l-41.2 41.2h-58.248l-20.623-20.626-20.622-20.622v-58.204l41.245-41.2h58.253l20.622 20.622 20.622 20.622zm-36.766-110.142h-67.111l-47.461 47.461v67.121l47.461 47.451h67.121l47.451-47.461v-67.111z"/><path class="b" d="M6352.434 4194.463l-41.2-41.2v-58.249l41.2-41.2h58.253l41.2 41.2v58.253l-41.2 41.2z"/><path class="a" d="M6345.138 4138.872h-14.73l-2.946 13.173h-11.038l15.054-55.4h13.061l15.054 55.386h-11.509zm-2.21-9.771l-1.139-5.1q-1.035-4.156-1.964-8.835t-1.964-9.005h-.295q-.884 4.419-1.807 9.044t-1.963 8.84l-1.178 5.1z"/><path class="a" d="M67 54.5h16.439a35.863 35.863 0 0 1 6.972.638 15.271 15.271 0 0 1 5.607 2.249 11.313 11.313 0 0 1 3.761 4.252 14.328 14.328 0 0 1 1.365 6.629 15 15 0 0 1-.481 3.741 13.228 13.228 0 0 1-1.443 3.491 11.027 11.027 0 0 1-2.514 2.887 9.947 9.947 0 0 1-3.614 1.866v.344a13.4 13.4 0 0 1 7.787 4.252q2.622 3.142 2.622 8.75a17.018 17.018 0 0 1-1.444 7.306 13.748 13.748 0 0 1-3.987 5.057 17.224 17.224 0 0 1-5.94 2.938 26.3 26.3 0 0 1-7.306.982h-17.824zm15.918 22.341q3.987 0 5.794-1.827a6.756 6.756 0 0 0 1.856-4.969q0-3.142-1.846-4.507t-5.686-1.355h-5.236v12.658zm1.041 23.323q8.926 0 8.926-7.562 0-3.653-2.21-5.313t-6.717-1.66h-6.158v14.534z" transform="translate(6299.34 4042.145)"/><path class="a" d="M111.78 54.5h17.568a29.107 29.107 0 0 1 7.345.894 15.457 15.457 0 0 1 5.98 2.946 14.219 14.219 0 0 1 4.026 5.519 21.447 21.447 0 0 1 1.473 8.455 21.447 21.447 0 0 1-1.512 8.406 16.105 16.105 0 0 1-4.1 5.892 16.7 16.7 0 0 1-6.02 3.4 23.225 23.225 0 0 1-7.237 1.1h-6.717v18.776h-10.8zm16.91 26.681q8.838 0 8.838-8.838 0-4.331-2.249-6.118t-6.589-1.787h-6.049v16.694z" transform="translate(6298.534 4042.145)"/><path class="c" d="M6415.117 4043.125h-67.111l-47.461 47.461v67.121l47.461 47.451h67.121l47.451-47.461v-67.111zm44.191 113.236l-45.526 45.556h-64.437l-45.559-45.556v-64.436l45.559-45.559h64.43l45.562 45.559z"/></svg> On 2017/11/03 17:39:59, Thomas Greiner wrote: > Can't we just use the existing Adblock Plus logo SVG file? The only difference > appears to be that this one has a slightly thicker gray border. This has not been addressed yet. https://codereview.adblockplus.org/29592569/diff/29596582/skin/updates.css File skin/updates.css (right): https://codereview.adblockplus.org/29592569/diff/29596582/skin/updates.css#ne... skin/updates.css:131: left: 100px; On 2017/11/06 16:05:45, martin wrote: > On 2017/11/03 17:40:00, Thomas Greiner wrote: > > Detail: This may produce unexpected results for right-to-left languages. > > Done. How did you address this? https://codereview.adblockplus.org/29592569/diff/29596582/updates.html File updates.html (right): https://codereview.adblockplus.org/29592569/diff/29596582/updates.html#newcode22 updates.html:22: <meta name="viewport" content="width=device-width, initial-scale=1" /> On 2017/11/06 16:05:50, martin wrote: > On 2017/11/03 17:40:02, Thomas Greiner wrote: > > I'm fine with adding this to have a more pleasant experience on mobile devices > > although the design is a bit broken because the text "Adblock Plus has been > > updated!" is oddly placed on smaller screens. Just wanted to mention this in > > case you haven't been aware of this breakage in the design. > > This page would never be viewed from a mobile device, so I don't think > optimising for mobile should be a priority, however I included a couple of CSS > rules to fix the breakage anyways. The spec doesn't mention that this should only appear in Chrome but yeah, practically, it will never be seen except for Firefox Mobile users who haven't received the web extension update yet. So I agree, let's keep it as is. https://codereview.adblockplus.org/29592569/diff/29596582/updates.html#newcode79 updates.html:79: <img src="skin/updates-page/googleplay-bg.svg" alt="google play store button"> On 2017/11/06 16:05:50, martin wrote: > On 2017/11/03 17:40:01, Thomas Greiner wrote: > > Are we allowed to use those images? Because according to > > https://play.google.com/intl/en_us/badges/ we're not allowed to alter the > badges > > and presumably, Apple's guidelines have similar requirements. > > Apple store button is in accordance with their guidelines. I exchanged the > monochrome graphic with a coloured one for the Google Play button and changed > the text. It now adheres to the guidelines. Great, thanks! https://codereview.adblockplus.org/29592569/diff/29605738/locale/en_US/update... File locale/en_US/updates.json (right): https://codereview.adblockplus.org/29592569/diff/29605738/locale/en_US/update... locale/en_US/updates.json:6: "message": "Update Complete" Detail: These two strings are the same so let's get rid of one and take the other in both cases. If necessary, we could use `text-transform` to apply the appropriate capitalization but I strongly assume that we want those two strings to look the same. https://codereview.adblockplus.org/29592569/diff/29605738/skin/updates.css File skin/updates.css (right): https://codereview.adblockplus.org/29592569/diff/29605738/skin/updates.css#ne... skin/updates.css:51: Coding style: "No whitespace at the end of a line." See https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style... https://codereview.adblockplus.org/29592569/diff/29605738/skin/updates.css#ne... skin/updates.css:79: #content header[dir="rtl"] This won't work because the "dir" attribute is on the `<body>` element. Same applies to all other occurrences where you used it like this. Note that you can easily test whether it works by changing the "dir" attribute because the browser will then change the layout of the page accordingly and you'll see which things might look odd. https://codereview.adblockplus.org/29592569/diff/29605738/skin/updates.css#ne... skin/updates.css:81: margin: 0 5em 2em 0; Coding style: "No whitespace at the end of a line." See https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style... https://codereview.adblockplus.org/29592569/diff/29605738/skin/updates.css#ne... skin/updates.css:102: margin: 0 0 0 3.4em; Detail: This may produce unexpected results for right-to-left languages. https://codereview.adblockplus.org/29592569/diff/29605738/skin/updates.css#ne... skin/updates.css:109: background-image: url(./updates/base-graphic.svg); Coding style: "CSS rule declaration order should follow the WordPress CSS Coding Standards." See https://adblockplus.org/en/coding-style#html-css and https://make.wordpress.org/core/handbook/best-practices/coding-standards/css/... https://codereview.adblockplus.org/29592569/diff/29605738/skin/updates.css#ne... skin/updates.css:135: margin: 0 1em 0 1em; Well spotted https://codereview.adblockplus.org/29592569/diff/29605738/skin/updates.css#ne... skin/updates.css:171: width: 100%; This causes both scrollbars to be shown when I open the page. Note that it's a block element anyway so by default it's already occupying the entire width so there shouldn't be a need for explicitly setting width to `100%`. https://codereview.adblockplus.org/29592569/diff/29605738/skin/updates.css#ne... skin/updates.css:225: margin: 2em 0 2em 5em; Detail: This may produce unexpected results for right-to-left languages. https://codereview.adblockplus.org/29592569/diff/29605738/skin/updates.css#ne... skin/updates.css:231: } Coding style: "Newline at end of file, otherwise no trailing whitespace." See https://adblockplus.org/en/coding-style#general Also applies to other files. Note that you can usually configure this in the text editor settings so that it automatically adds it if it's missing. Same goes for other settings (see https://hg.adblockplus.org/codingtools/file/tip/editorconfig). https://codereview.adblockplus.org/29592569/diff/29605738/updates.html File updates.html (right): https://codereview.adblockplus.org/29592569/diff/29605738/updates.html#newcode16 updates.html:16: --> Please leave the license header unchanged. Otherwise it could turn out to be more difficult to update it across all files in the future. https://codereview.adblockplus.org/29592569/diff/29605738/updates.html#newcode58 updates.html:58: <p class="i18n_updates_optionsPageNews"><a href="#" class="i18n_updates_optionsPageLink"></a></p> There are still a couple of links missing on this page. Note that in this case we shouldn't just link to the options page but we need to call `require("options").showOptions()` so that the extension opens it for us. It focuses our options page if it's already open and it makes sure that the correct options page is opened. https://codereview.adblockplus.org/29592569/diff/29605738/updates.html#newcode72 updates.html:72: <div class="store-buttons"> Have we made a decision regarding the store buttons yet? From what I remember, Jeen and Tamara would be in favor of the WhatsApp approach but I don't think we concluded that IRC discussion yet.
Pushed a new review, addressing (almost all) comments from the previous one. https://codereview.adblockplus.org/29592569/diff/29596582/skin/updates-page/a... File skin/updates-page/abp-logo.svg (right): https://codereview.adblockplus.org/29592569/diff/29596582/skin/updates-page/a... skin/updates-page/abp-logo.svg:1: <svg xmlns="http://www.w3.org/2000/svg" viewBox="6300.545 4043.125 162.032 162.032"><defs><style>.a{fill:#fff;}.b{fill:#c70d2c;}.c{fill:#d2d2d2;}</style></defs><path class="a" d="M6438.852 4066.85l-23.735-23.725h-67.111l-47.461 47.461v67.121l23.725 23.725 23.735 23.725h67.121l47.451-47.461v-67.11zm13.031 86.417l-41.2 41.2h-58.248l-20.623-20.626-20.622-20.622v-58.204l41.245-41.2h58.253l20.622 20.622 20.622 20.622zm-36.766-110.142h-67.111l-47.461 47.461v67.121l47.461 47.451h67.121l47.451-47.461v-67.111z"/><path class="b" d="M6352.434 4194.463l-41.2-41.2v-58.249l41.2-41.2h58.253l41.2 41.2v58.253l-41.2 41.2z"/><path class="a" d="M6345.138 4138.872h-14.73l-2.946 13.173h-11.038l15.054-55.4h13.061l15.054 55.386h-11.509zm-2.21-9.771l-1.139-5.1q-1.035-4.156-1.964-8.835t-1.964-9.005h-.295q-.884 4.419-1.807 9.044t-1.963 8.84l-1.178 5.1z"/><path class="a" d="M67 54.5h16.439a35.863 35.863 0 0 1 6.972.638 15.271 15.271 0 0 1 5.607 2.249 11.313 11.313 0 0 1 3.761 4.252 14.328 14.328 0 0 1 1.365 6.629 15 15 0 0 1-.481 3.741 13.228 13.228 0 0 1-1.443 3.491 11.027 11.027 0 0 1-2.514 2.887 9.947 9.947 0 0 1-3.614 1.866v.344a13.4 13.4 0 0 1 7.787 4.252q2.622 3.142 2.622 8.75a17.018 17.018 0 0 1-1.444 7.306 13.748 13.748 0 0 1-3.987 5.057 17.224 17.224 0 0 1-5.94 2.938 26.3 26.3 0 0 1-7.306.982h-17.824zm15.918 22.341q3.987 0 5.794-1.827a6.756 6.756 0 0 0 1.856-4.969q0-3.142-1.846-4.507t-5.686-1.355h-5.236v12.658zm1.041 23.323q8.926 0 8.926-7.562 0-3.653-2.21-5.313t-6.717-1.66h-6.158v14.534z" transform="translate(6299.34 4042.145)"/><path class="a" d="M111.78 54.5h17.568a29.107 29.107 0 0 1 7.345.894 15.457 15.457 0 0 1 5.98 2.946 14.219 14.219 0 0 1 4.026 5.519 21.447 21.447 0 0 1 1.473 8.455 21.447 21.447 0 0 1-1.512 8.406 16.105 16.105 0 0 1-4.1 5.892 16.7 16.7 0 0 1-6.02 3.4 23.225 23.225 0 0 1-7.237 1.1h-6.717v18.776h-10.8zm16.91 26.681q8.838 0 8.838-8.838 0-4.331-2.249-6.118t-6.589-1.787h-6.049v16.694z" transform="translate(6298.534 4042.145)"/><path class="c" d="M6415.117 4043.125h-67.111l-47.461 47.461v67.121l47.461 47.451h67.121l47.451-47.461v-67.111zm44.191 113.236l-45.526 45.556h-64.437l-45.559-45.556v-64.436l45.559-45.559h64.43l45.562 45.559z"/></svg> On 2017/11/14 12:25:38, Thomas Greiner wrote: > On 2017/11/03 17:39:59, Thomas Greiner wrote: > > Can't we just use the existing Adblock Plus logo SVG file? The only difference > > appears to be that this one has a slightly thicker gray border. > > This has not been addressed yet. Oops. Forgotten about this one. Now using the existing ABP logo. Deleted the old one. https://codereview.adblockplus.org/29592569/diff/29596582/skin/updates.css File skin/updates.css (right): https://codereview.adblockplus.org/29592569/diff/29596582/skin/updates.css#ne... skin/updates.css:131: left: 100px; On 2017/11/14 12:25:38, Thomas Greiner wrote: > On 2017/11/06 16:05:45, martin wrote: > > On 2017/11/03 17:40:00, Thomas Greiner wrote: > > > Detail: This may produce unexpected results for right-to-left languages. > > > > Done. > > How did you address this? I checked it out and I guess it looks all right in "rtl". Let me know if you had something else in mind. https://codereview.adblockplus.org/29592569/diff/29596582/skin/updates.css#ne... skin/updates.css:163: margin: 0 1em 0 0; On 2017/11/03 17:40:00, Thomas Greiner wrote: > Detail: This may produce unexpected results for right-to-left languages. Done. https://codereview.adblockplus.org/29592569/diff/29605738/locale/en_US/update... File locale/en_US/updates.json (right): https://codereview.adblockplus.org/29592569/diff/29605738/locale/en_US/update... locale/en_US/updates.json:6: "message": "Update Complete" On 2017/11/14 12:25:40, Thomas Greiner wrote: > Detail: These two strings are the same so let's get rid of one and take the > other in both cases. If necessary, we could use `text-transform` to apply the > appropriate capitalization but I strongly assume that we want those two strings > to look the same. I removed updates_title. Kept updates_status. I didn't `transform: uppercase` the title because I'm actually not a fan of tabs that shout at you for no apparent reason. https://codereview.adblockplus.org/29592569/diff/29605738/skin/updates.css File skin/updates.css (right): https://codereview.adblockplus.org/29592569/diff/29605738/skin/updates.css#ne... skin/updates.css:51: On 2017/11/14 12:25:43, Thomas Greiner wrote: > Coding style: "No whitespace at the end of a line." > See > https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style... Done. https://codereview.adblockplus.org/29592569/diff/29605738/skin/updates.css#ne... skin/updates.css:79: #content header[dir="rtl"] On 2017/11/14 12:25:41, Thomas Greiner wrote: > This won't work because the "dir" attribute is on the `<body>` element. > > Same applies to all other occurrences where you used it like this. > > Note that you can easily test whether it works by changing the "dir" attribute > because the browser will then change the layout of the page accordingly and > you'll see which things might look odd. Done. https://codereview.adblockplus.org/29592569/diff/29605738/skin/updates.css#ne... skin/updates.css:81: margin: 0 5em 2em 0; On 2017/11/14 12:25:42, Thomas Greiner wrote: > Coding style: "No whitespace at the end of a line." > See > https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style... Done. https://codereview.adblockplus.org/29592569/diff/29605738/skin/updates.css#ne... skin/updates.css:102: margin: 0 0 0 3.4em; On 2017/11/14 12:25:42, Thomas Greiner wrote: > Detail: This may produce unexpected results for right-to-left languages. Done. https://codereview.adblockplus.org/29592569/diff/29605738/skin/updates.css#ne... skin/updates.css:109: background-image: url(./updates/base-graphic.svg); On 2017/11/14 12:25:41, Thomas Greiner wrote: > Coding style: "CSS rule declaration order should follow the WordPress CSS Coding > Standards." > See https://adblockplus.org/en/coding-style#html-css and > https://make.wordpress.org/core/handbook/best-practices/coding-standards/css/... Soo... Is this about the declaration order? Should I alphabetise? https://codereview.adblockplus.org/29592569/diff/29605738/skin/updates.css#ne... skin/updates.css:171: width: 100%; On 2017/11/14 12:25:41, Thomas Greiner wrote: > This causes both scrollbars to be shown when I open the page. Note that it's a > block element anyway so by default it's already occupying the entire width so > there shouldn't be a need for explicitly setting width to `100%`. Done. https://codereview.adblockplus.org/29592569/diff/29605738/skin/updates.css#ne... skin/updates.css:225: margin: 2em 0 2em 5em; On 2017/11/14 12:25:42, Thomas Greiner wrote: > Detail: This may produce unexpected results for right-to-left languages. Previewed it. Saw nothing weird. Let me know if I'm missing something. https://codereview.adblockplus.org/29592569/diff/29605738/skin/updates.css#ne... skin/updates.css:231: } On 2017/11/14 12:25:41, Thomas Greiner wrote: > Coding style: "Newline at end of file, otherwise no trailing whitespace." > See https://adblockplus.org/en/coding-style#general > > Also applies to other files. > > Note that you can usually configure this in the text editor settings so that it > automatically adds it if it's missing. Same goes for other settings (see > https://hg.adblockplus.org/codingtools/file/tip/editorconfig). Done. https://codereview.adblockplus.org/29592569/diff/29605738/updates.html File updates.html (right): https://codereview.adblockplus.org/29592569/diff/29605738/updates.html#newcode16 updates.html:16: --> On 2017/11/14 12:25:43, Thomas Greiner wrote: > Please leave the license header unchanged. Otherwise it could turn out to be > more difficult to update it across all files in the future. Done. https://codereview.adblockplus.org/29592569/diff/29605738/updates.html#newcode58 updates.html:58: <p class="i18n_updates_optionsPageNews"><a href="#" class="i18n_updates_optionsPageLink"></a></p> On 2017/11/14 12:25:43, Thomas Greiner wrote: > There are still a couple of links missing on this page. > > Note that in this case we shouldn't just link to the options page but we need to > call `require("options").showOptions()` so that the extension opens it for us. > It focuses our options page if it's already open and it makes sure that the > correct options page is opened. Hmm... I took a look and couldn't figure that one out. Are we even using require.js or ES6 modules? Is there an "options" module? Anyways I'd better leave this to to you guys. Would only slow you down. https://codereview.adblockplus.org/29592569/diff/29605738/updates.html#newcode72 updates.html:72: <div class="store-buttons"> On 2017/11/14 12:25:44, Thomas Greiner wrote: > Have we made a decision regarding the store buttons yet? From what I remember, > Jeen and Tamara would be in favor of the WhatsApp approach but I don't think we > concluded that IRC discussion yet. Done.
There are mostly JavaScript related issues left. https://codereview.adblockplus.org/29592569/diff/29596582/skin/updates.css File skin/updates.css (right): https://codereview.adblockplus.org/29592569/diff/29596582/skin/updates.css#ne... skin/updates.css:131: left: 100px; On 2017/11/20 11:47:28, martin wrote: > On 2017/11/14 12:25:38, Thomas Greiner wrote: > > On 2017/11/06 16:05:45, martin wrote: > > > On 2017/11/03 17:40:00, Thomas Greiner wrote: > > > > Detail: This may produce unexpected results for right-to-left languages. > > > > > > Done. > > > > How did you address this? > > I checked it out and I guess it looks all right in "rtl". Let me know if you had > something else in mind. I looked at the page now and see that this is not an issue simply because the background image isn't mirrored. So you're right, it should be fine. https://codereview.adblockplus.org/29592569/diff/29605738/locale/en_US/update... File locale/en_US/updates.json (right): https://codereview.adblockplus.org/29592569/diff/29605738/locale/en_US/update... locale/en_US/updates.json:6: "message": "Update Complete" On 2017/11/20 11:47:30, martin wrote: > On 2017/11/14 12:25:40, Thomas Greiner wrote: > > Detail: These two strings are the same so let's get rid of one and take the > > other in both cases. If necessary, we could use `text-transform` to apply the > > appropriate capitalization but I strongly assume that we want those two > strings > > to look the same. > > I removed updates_title. Kept updates_status. I didn't `transform: uppercase` > the title because I'm actually not a fan of tabs that shout at you for no > apparent reason. Looks good now. Note that I was referring to `text-transform: capitalize`, not `text-transform: uppercase`. https://codereview.adblockplus.org/29592569/diff/29605738/skin/updates.css File skin/updates.css (right): https://codereview.adblockplus.org/29592569/diff/29605738/skin/updates.css#ne... skin/updates.css:1: @font-face I'm glad that you got rid of those because it's pretty much impossible for me to spot them during a review. https://codereview.adblockplus.org/29592569/diff/29605738/skin/updates.css#ne... skin/updates.css:109: background-image: url(./updates/base-graphic.svg); On 2017/11/20 11:47:30, martin wrote: > On 2017/11/14 12:25:41, Thomas Greiner wrote: > > Coding style: "CSS rule declaration order should follow the WordPress CSS > Coding > > Standards." > > See https://adblockplus.org/en/coding-style#html-css and > > > https://make.wordpress.org/core/handbook/best-practices/coding-standards/css/... > > Soo... Is this about the declaration order? Should I alphabetise? Yes, it's about the order of declaration. No, don't sort them alphabetically but grouped by what's defined in the linked Wordpress coding style. So in this case the ordering should be as follows: 1. Display: display 2. Positioning: align-items, position (in any order) 3. Box model: height, width (in any order) 4. Colors and typography: background 5. Other: N/A https://codereview.adblockplus.org/29592569/diff/29605738/skin/updates.css#ne... skin/updates.css:225: margin: 2em 0 2em 5em; On 2017/11/20 11:47:33, martin wrote: > On 2017/11/14 12:25:42, Thomas Greiner wrote: > > Detail: This may produce unexpected results for right-to-left languages. > > Previewed it. Saw nothing weird. Let me know if I'm missing something. I'd have to check whether this issue is visible. But in general, you use "margin-left" here which will not be applied correctly in right-to-left environments. Therefore we need to handle this even if we don't see anything wrong with it so that it doesn't bite us later on. https://codereview.adblockplus.org/29592569/diff/29605738/updates.html File updates.html (right): https://codereview.adblockplus.org/29592569/diff/29605738/updates.html#newcode58 updates.html:58: <p class="i18n_updates_optionsPageNews"><a href="#" class="i18n_updates_optionsPageLink"></a></p> On 2017/11/20 11:47:34, martin wrote: > On 2017/11/14 12:25:43, Thomas Greiner wrote: > > There are still a couple of links missing on this page. > > > > Note that in this case we shouldn't just link to the options page but we need > to > > call `require("options").showOptions()` so that the extension opens it for us. > > It focuses our options page if it's already open and it makes sure that the > > correct options page is opened. > > Hmm... I took a look and couldn't figure that one out. Are we even using > require.js or ES6 modules? Is there an "options" module? Anyways I'd better > leave this to to you guys. Would only slow you down. It should be fine for you to include it here. We simply need to call `browser.runtime.sendMessage({type: "app.open", what: "options"});` when someone clicks on the link. FYI: `require("options").showOptions()` can't be used by the UI so that was my mistake and you can ignore that. https://codereview.adblockplus.org/29592569/diff/29612630/skin/updates.css File skin/updates.css (right): https://codereview.adblockplus.org/29592569/diff/29612630/skin/updates.css#ne... skin/updates.css:220: margin: 0 0 0 1em; Detail: As I mentioned in one of my previous comments, we should avoid overriding styles that we don't care about (i.e. "margin-top" and "margin-bottom") to avoid unexpected behavior. For that, the previous approach worked better. I'd recommend checking each "margin" accoredingly. https://codereview.adblockplus.org/29592569/diff/29612630/skin/updates.css#ne... skin/updates.css:237: .column Coding style: "Separate rules by new lines." See https://adblockplus.org/en/coding-style#html-css and https://google.github.io/styleguide/htmlcssguide.html#Rule_Separation https://codereview.adblockplus.org/29592569/diff/29612630/skin/updates/google... File skin/updates/googleplay-bg.svg (right): https://codereview.adblockplus.org/29592569/diff/29612630/skin/updates/google... skin/updates/googleplay-bg.svg:1: <svg width="180" height="54" viewBox="0 0 180 54" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"><defs><linearGradient x1="58.917%" y1="9.59%" x2="90.81%" y2="60.851%" id="a"><stop stop-color="#FF177B" offset="0%"/><stop stop-color="#FFEC73" offset="100%"/></linearGradient><linearGradient x1="37.103%" y1="-44.035%" x2="0%" y2="101.06%" id="c"><stop stop-color="#064AA2" offset="0%"/><stop stop-color="#63FFD4" offset="100%"/></linearGradient><path d="M14.355 13.417l-13.839-13.417c-.323.077-.516.324-.516.74v25.673000000000002c0 .353.139.563.31.634l14.046-13.629z" id="b"/><path d="M.572 13.5l7.999-4.495 3.533-2.003 5.817-3.205-3.873-3.849-14.045 13.63c.157.052.432.023.648-.085" id="e"/><linearGradient x1="78.645%" y1="50%" x2="2.495%" y2="100%" id="f"><stop stop-color="#FF4521" offset="0%"/><stop stop-color="#8A33DB" offset="100%"/></linearGradient><linearGradient x1="0%" y1="-24.186%" x2="64.224%" y2="100%" id="i"><stop stop-color="#00A3B5" offset="0%"/><stop stop-color="#D6FFA1" offset="100%"/></linearGradient><path d="M.736.121c-.266-.146-.531-.188-.745-.129l13.851 13.437 3.832-3.709-5.776-3.273s-5.444-3.085-8.731-4.949l-2.43-1.377z" id="h"/></defs><g fill="none"><rect fill="#000" fill-rule="nonzero" width="179.719" height="54" rx="8.438"/><path d="M123.956 40.78l-1.269 1.22c-.293.164-.585.355-.876.49-.881.408-1.778.51-2.574.51-.847 0-2.176-.056-3.531-1.055-1.883-1.351-2.706-3.674-2.706-5.699 0-4.191 3.348-6.246 6.079-6.246.954 0 1.935.244 2.732.757 1.325.893 1.668 2.054 1.858 2.675l-6.237 2.567-2.042.162c.661 3.436 2.943 5.435 5.464 5.435 1.259 0 2.194-.419 3.052-.851 0 0 .172-.091.05.034zm-3.811-6.894c.501-.189.761-.352.761-.731 0-1.087-1.185-2.343-2.6-2.343-1.048 0-3.015.847-3.015 3.787 0 .459.053.948.079 1.436l4.774-2.15z" id="Fill-1" fill="#fff"/><path d="M110.593 39.985c0 1.046.184 1.208 1.054 1.29l1.353.132-.979.593h-4.667c.612-.807.717-.888.717-1.425v-.601l-.01-16.003h-2.061l1.984-.971h3.787c-.821.483-1.066.778-1.172 1.72l-.006 15.265" id="Fill-2" fill="#fff"/><path d="M104.461 30.996c.644.52 1.988 1.624 1.988 3.718 0 2.041-1.171 3.003-2.352 3.914-.362.359-.786.747-.786 1.348 0 .61.424.941.729 1.187l1.013.768c1.232 1.022 2.355 1.958 2.355 3.861 0 2.592-2.553 5.206-7.378 5.206-4.064 0-6.029-1.902-6.029-3.941 0-.992.503-2.398 2.162-3.361 1.739-1.045 4.094-1.184 5.356-1.266-.394-.5-.844-1.023-.844-1.878 0-.467.143-.746.281-1.074-.307.029-.616.058-.897.058-2.975 0-4.656-2.181-4.656-4.329 0-1.267.588-2.674 1.797-3.693 1.599-1.294 3.506-1.517 5.021-1.517h5.78l-1.797.996h-1.742zm-1.973 12.203c-.232-.03-.371-.03-.651-.03-.256 0-1.784.055-2.967.449-.626.227-2.435.898-2.435 2.891 0 1.989 1.953 3.423 4.975 3.423 2.715 0 4.156-1.291 4.156-3.03 0-1.431-.933-2.186-3.078-3.703zm.793-5.308c.646-.634.702-1.51.702-2.008 0-1.976-1.198-5.053-3.519-5.053-.724 0-1.509.357-1.954.91-.473.572-.614 1.315-.614 2.029 0 1.84 1.091 4.891 3.49 4.891.7 0 1.453-.335 1.895-.769z" id="Fill-3" fill="#fff"/><path d="M86.858 43c-4.468 0-6.858-3.36-6.858-6.403 0-3.555 3.005-6.597 7.28-6.597 4.132 0 6.72 3.125 6.72 6.405 0 3.204-2.558 6.595-7.142 6.595zm3.494-2.198c.682-.871.847-1.959.847-3.019 0-2.398-1.183-6.971-4.681-6.971-.93 0-1.867.351-2.542.924-1.102.953-1.298 2.15-1.298 3.325 0 2.69 1.377 7.126 4.796 7.126 1.103 0 2.229-.516 2.878-1.385z" id="Fill-4" fill="#fff"/><path d="M72.858 43c-4.471 0-6.858-3.36-6.858-6.403 0-3.555 3.007-6.597 7.282-6.597 4.131 0 6.718 3.125 6.718 6.405 0 3.204-2.556 6.595-7.142 6.595zm3.496-2.198c.677-.871.846-1.959.846-3.019 0-2.398-1.187-6.971-4.684-6.971-.934 0-1.861.351-2.539.924-1.104.953-1.299 2.15-1.299 3.325 0 2.69 1.384 7.126 4.798 7.126 1.101 0 2.226-.516 2.877-1.385z" id="Fill-5" fill="#fff"/><path d="M64.011 42.711l-3.738.845c-1.515.236-2.875.443-4.315.443-7.21 0-9.958-5.242-9.958-9.345 0-5.01 3.893-9.655 10.559-9.655 1.41 0 2.767.205 4.003.538 1.959.544 2.876 1.214 3.448 1.606l-2.172 2.037-.916.202.653-1.027c-.884-.853-2.509-2.427-5.594-2.427-4.124 0-7.235 3.096-7.235 7.617 0 4.855 3.552 9.422 9.25 9.422 1.674 0 2.536-.332 3.318-.642v-4.162l-3.947.21 2.092-1.11h5.54l-.678.644c-.183.153-.207.206-.259.411-.027.235-.053.983-.053 1.245v3.148" id="Fill-6" fill="#fff"/><path d="M132.433 41.052v6.948h-1.433v-17.681h1.433v2.014c.933-1.302 2.639-2.333 4.62-2.333 3.553 0 5.947 2.596 5.947 6.705 0 4.082-2.393 6.732-5.947 6.732-1.87 0-3.546-.929-4.62-2.385zm9.098-4.241c0-3.13-1.683-5.517-4.666-5.517-1.853 0-3.625 1.412-4.395 2.643v5.719c.77 1.234 2.542 2.697 4.395 2.697 2.984 0 4.666-2.408 4.666-5.542z" id="Fill-7" fill="#fff"/><path fill="#fff" d="M144 25h1v18h-1z"/><path d="M161.31 46.541c.286.132.774.213 1.084.213.818 0 1.388-.345 1.899-1.565l.976-2.281-5.269-12.908h1.492l4.519 11.186 4.476-11.186h1.513l-6.426 15.617c-.667 1.609-1.716 2.383-3.157 2.383-.433 0-1.003-.079-1.337-.185l.23-1.274" fill="#fff"/><path d="M157.396 42.755c-.093-.433-.163-.814-.214-1.132-.042-.318-.067-.642-.067-.972-.477.677-1.153 1.24-2.017 1.684-.864.447-1.686.666-2.812.666-1.356 0-2.409-.322-3.16-.976-.749-.653-1.125-1.538-1.125-2.658 0-1.118.546-2.026 1.634-2.72 1.088-.697 2.494-1.044 4.234-1.044h3.247v-1.55c0-.893-.315-1.595-.944-2.104-.631-.51-1.517-.767-2.665-.767-1.05 0-1.901.236-2.549.694-.643.468-.966 1.048-.966 1.747h-1.425l-.029-.067c-.051-.942.395-1.769 1.343-2.483.944-.713 2.182-1.072 3.704-1.072 1.518 0 2.746.353 3.672 1.057.925.697 1.389 1.705 1.389 3.023v6.154c0 .442.022.869.079 1.285.059.417.153.833.277 1.236h-1.604zm-4.943-.895c1.182 0 2.031-.257 2.898-.764.862-.505 1.449-1.163 1.756-1.959v-2.408h-3.259c-1.245 0-2.276.268-3.087.808-.812.539-1.224 1.19-1.224 1.948 0 .715.261 1.287.779 1.72.52.438 1.23.655 2.137.655z" fill="#fff"/><path d="M46 12.995c0-2.431 2-3.995 4.487-3.995 1.667 0 2.757.718 3.436 1.598l-1.217.637c-.461-.579-1.27-1.03-2.219-1.03-1.692 0-2.948 1.157-2.948 2.79 0 1.609 1.257 2.79 2.948 2.79.872 0 1.641-.359 2.026-.694v-1.193h-2.564v-1.193h4.051v2.883c-.833.845-2.038 1.413-3.513 1.413-2.487 0-4.487-1.585-4.487-4.005" id="Fill-1" fill="#fff"/><path fill="#fff" d="M56 17v-8h6v1.236h-4.464v2.062h4.372v1.236h-4.372v2.231h4.464v1.236h-6"/><path fill="#fff" d="M65.714 17v-6.764h-2.714v-1.236h7v1.236h-2.714v6.764h-1.572"/><path id="Fill-4" fill="#fff" d="M75 9h1v8h-1z"/><path fill="#fff" d="M80.715 17v-6.764h-2.715v-1.236h7v1.236h-2.713v6.764h-1.572"/><path d="M90 13c0-2.308 1.861-4 4.5-4 2.625 0 4.5 1.693 4.5 4s-1.875 4-4.5 4c-2.64 0-4.5-1.693-4.5-4zm7.397-.059c0-1.583-1.14-2.765-2.897-2.765-1.77 0-2.898 1.181-2.898 2.765 0 1.571 1.128 2.765 2.898 2.765 1.757 0 2.897-1.193 2.897-2.765z" fill="#fff"/><path id="Fill-7" fill="#fff" d="M107.436 17l-4.817-5.721v5.721h-1.619v-8h1.661l4.719 5.542v-5.542h1.62v8h-1.564"/><g><path d="M25.412 27.295s-10.524-5.965-11.143-6.315c-.619-.35-1.235-.138-1.235.595v25.727c0 .548.44.786.978.482l11.4-6.462 2.214-1.254 3.562-2.018 5.553-3.147c.651-.369.588-.876.039-1.167-.55-.29-5.592-3.168-5.592-3.168l-5.776-3.273z" fill="url(#a)"/><g transform="translate(13 20) translate(0 .848)"><mask id="d"><use xlink:href="#b"/></mask><use fill="url(#c)" xlink:href="#b"/><path d="M12.393 6.445c-.02.048-10.541-5.928-11.154-6.233-.623-.397-1.239-.184-1.239.528v25.778c0 .566.44.805.929.528.585-.333 11.443-6.502 11.464-6.55l5.783-3.275c-.03.046 4.869-2.738 5.474-3.064.698-.413.635-.92.103-1.162-.567-.341-5.607-3.224-5.577-3.275l-5.783-3.275z" mask="url(#d)"/></g><g transform="translate(13 20) translate(.308 14.319)"><mask id="g"><use xlink:href="#e"/></mask><path d="M12.085-7.026c-.02.048-10.541-5.928-11.154-6.233-.623-.397-1.239-.184-1.239.528v25.778c0 .566.44.805.929.528.585-.333 11.443-6.502 11.464-6.55l5.783-3.275c-.03.046 4.869-2.738 5.474-3.064.698-.413.635-.92.103-1.162-.567-.341-5.607-3.224-5.577-3.275l-5.783-3.275z" fill="url(#f)" mask="url(#g)"/></g><g transform="translate(13 20) translate(.514 .848)"><mask id="j"><use xlink:href="#h"/></mask><use fill="url(#i)" xlink:href="#h"/><path d="M17.662 9.72l-5.783-3.275c-.02.048-10.541-5.928-11.154-6.233-.623-.397-1.239-.184-1.239.528v25.778c0 .566.44.805.929.528.585-.333 11.443-6.502 11.464-6.55l5.783-3.275c-.03.046 4.869-2.738 5.474-3.064.698-.413.635-.92.103-1.162-.567-.341-5.607-3.224-5.577-3.275" mask="url(#j)"/></g></g></g></svg> This image is quite outdated seeing that the colors are less saturated and the Google logo is quite old when comparing it to the one from https://play.google.com/intl/en_us/badges/ Let's also update the App Store logo to a newer versions in case we also went with an old version for that one. https://codereview.adblockplus.org/29592569/diff/29612630/updates.html File updates.html (right): https://codereview.adblockplus.org/29592569/diff/29612630/updates.html#newcode30 updates.html:30: <script src="updates.js"></script> This file is not included in the review so please make sure to add it. https://codereview.adblockplus.org/29592569/diff/29612630/updates.html#newcode60 updates.html:60: <p id="optionsPageNews" class="i18n_updates_optionsPageNews"></p> Coding style: "Separate words in ID and class names by a hyphen." See https://adblockplus.org/en/coding-style#html-css and https://google.github.io/styleguide/htmlcssguide.html#ID_and_Class_Name_Delim... https://codereview.adblockplus.org/29592569/diff/29612630/updates.html#newcode75 updates.html:75: <a href="https://eyeo.to/adblockbrowser/ios/update-page" class="store-button applestore-button"> We use documentation links (see https://bitbucket.org/adblockplus/spec/src/3a5d8435c857976db8e4b58d18148504f4...) so we set any links using JavaScript. Looks like those links haven't been added to the spec yet though so we need to make sure to add them before we roll out the update. Example: https://hg.adblockplus.org/adblockplusui/file/tip/mobile-options.js#l435
Sending a new patch your way. Let me know if I missed something. https://codereview.adblockplus.org/29592569/diff/29605738/skin/updates.css File skin/updates.css (right): https://codereview.adblockplus.org/29592569/diff/29605738/skin/updates.css#ne... skin/updates.css:109: background-image: url(./updates/base-graphic.svg); On 2017/11/20 17:06:04, Thomas Greiner wrote: > On 2017/11/20 11:47:30, martin wrote: > > On 2017/11/14 12:25:41, Thomas Greiner wrote: > > > Coding style: "CSS rule declaration order should follow the WordPress CSS > > Coding > > > Standards." > > > See https://adblockplus.org/en/coding-style#html-css and > > > > > > https://make.wordpress.org/core/handbook/best-practices/coding-standards/css/... > > > > Soo... Is this about the declaration order? Should I alphabetise? > > Yes, it's about the order of declaration. > No, don't sort them alphabetically but grouped by what's defined in the linked > Wordpress coding style. > > So in this case the ordering should be as follows: > > 1. Display: display > 2. Positioning: align-items, position (in any order) > 3. Box model: height, width (in any order) > 4. Colors and typography: background > 5. Other: N/A Done. https://codereview.adblockplus.org/29592569/diff/29605738/updates.html File updates.html (right): https://codereview.adblockplus.org/29592569/diff/29605738/updates.html#newcode58 updates.html:58: <p class="i18n_updates_optionsPageNews"><a href="#" class="i18n_updates_optionsPageLink"></a></p> On 2017/11/20 17:06:05, Thomas Greiner wrote: > On 2017/11/20 11:47:34, martin wrote: > > On 2017/11/14 12:25:43, Thomas Greiner wrote: > > > There are still a couple of links missing on this page. > > > > > > Note that in this case we shouldn't just link to the options page but we > need > > to > > > call `require("options").showOptions()` so that the extension opens it for > us. > > > It focuses our options page if it's already open and it makes sure that the > > > correct options page is opened. > > > > Hmm... I took a look and couldn't figure that one out. Are we even using > > require.js or ES6 modules? Is there an "options" module? Anyways I'd better > > leave this to to you guys. Would only slow you down. > > It should be fine for you to include it here. We simply need to call > `browser.runtime.sendMessage({type: "app.open", what: "options"});` when someone > clicks on the link. > > FYI: `require("options").showOptions()` can't be used by the UI so that was my > mistake and you can ignore that. Done. https://codereview.adblockplus.org/29592569/diff/29612630/skin/updates.css File skin/updates.css (right): https://codereview.adblockplus.org/29592569/diff/29612630/skin/updates.css#ne... skin/updates.css:220: margin: 0 0 0 1em; On 2017/11/20 17:06:06, Thomas Greiner wrote: > Detail: As I mentioned in one of my previous comments, we should avoid > overriding styles that we don't care about (i.e. "margin-top" and > "margin-bottom") to avoid unexpected behavior. > > For that, the previous approach worked better. > > I'd recommend checking each "margin" accoredingly. Done. https://codereview.adblockplus.org/29592569/diff/29612630/skin/updates.css#ne... skin/updates.css:237: .column On 2017/11/20 17:06:06, Thomas Greiner wrote: > Coding style: "Separate rules by new lines." > > See https://adblockplus.org/en/coding-style#html-css and > https://google.github.io/styleguide/htmlcssguide.html#Rule_Separation Done. https://codereview.adblockplus.org/29592569/diff/29612630/skin/updates/google... File skin/updates/googleplay-bg.svg (right): https://codereview.adblockplus.org/29592569/diff/29612630/skin/updates/google... skin/updates/googleplay-bg.svg:1: <svg width="180" height="54" viewBox="0 0 180 54" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"><defs><linearGradient x1="58.917%" y1="9.59%" x2="90.81%" y2="60.851%" id="a"><stop stop-color="#FF177B" offset="0%"/><stop stop-color="#FFEC73" offset="100%"/></linearGradient><linearGradient x1="37.103%" y1="-44.035%" x2="0%" y2="101.06%" id="c"><stop stop-color="#064AA2" offset="0%"/><stop stop-color="#63FFD4" offset="100%"/></linearGradient><path d="M14.355 13.417l-13.839-13.417c-.323.077-.516.324-.516.74v25.673000000000002c0 .353.139.563.31.634l14.046-13.629z" id="b"/><path d="M.572 13.5l7.999-4.495 3.533-2.003 5.817-3.205-3.873-3.849-14.045 13.63c.157.052.432.023.648-.085" id="e"/><linearGradient x1="78.645%" y1="50%" x2="2.495%" y2="100%" id="f"><stop stop-color="#FF4521" offset="0%"/><stop stop-color="#8A33DB" offset="100%"/></linearGradient><linearGradient x1="0%" y1="-24.186%" x2="64.224%" y2="100%" id="i"><stop stop-color="#00A3B5" offset="0%"/><stop stop-color="#D6FFA1" offset="100%"/></linearGradient><path d="M.736.121c-.266-.146-.531-.188-.745-.129l13.851 13.437 3.832-3.709-5.776-3.273s-5.444-3.085-8.731-4.949l-2.43-1.377z" id="h"/></defs><g fill="none"><rect fill="#000" fill-rule="nonzero" width="179.719" height="54" rx="8.438"/><path d="M123.956 40.78l-1.269 1.22c-.293.164-.585.355-.876.49-.881.408-1.778.51-2.574.51-.847 0-2.176-.056-3.531-1.055-1.883-1.351-2.706-3.674-2.706-5.699 0-4.191 3.348-6.246 6.079-6.246.954 0 1.935.244 2.732.757 1.325.893 1.668 2.054 1.858 2.675l-6.237 2.567-2.042.162c.661 3.436 2.943 5.435 5.464 5.435 1.259 0 2.194-.419 3.052-.851 0 0 .172-.091.05.034zm-3.811-6.894c.501-.189.761-.352.761-.731 0-1.087-1.185-2.343-2.6-2.343-1.048 0-3.015.847-3.015 3.787 0 .459.053.948.079 1.436l4.774-2.15z" id="Fill-1" fill="#fff"/><path d="M110.593 39.985c0 1.046.184 1.208 1.054 1.29l1.353.132-.979.593h-4.667c.612-.807.717-.888.717-1.425v-.601l-.01-16.003h-2.061l1.984-.971h3.787c-.821.483-1.066.778-1.172 1.72l-.006 15.265" id="Fill-2" fill="#fff"/><path d="M104.461 30.996c.644.52 1.988 1.624 1.988 3.718 0 2.041-1.171 3.003-2.352 3.914-.362.359-.786.747-.786 1.348 0 .61.424.941.729 1.187l1.013.768c1.232 1.022 2.355 1.958 2.355 3.861 0 2.592-2.553 5.206-7.378 5.206-4.064 0-6.029-1.902-6.029-3.941 0-.992.503-2.398 2.162-3.361 1.739-1.045 4.094-1.184 5.356-1.266-.394-.5-.844-1.023-.844-1.878 0-.467.143-.746.281-1.074-.307.029-.616.058-.897.058-2.975 0-4.656-2.181-4.656-4.329 0-1.267.588-2.674 1.797-3.693 1.599-1.294 3.506-1.517 5.021-1.517h5.78l-1.797.996h-1.742zm-1.973 12.203c-.232-.03-.371-.03-.651-.03-.256 0-1.784.055-2.967.449-.626.227-2.435.898-2.435 2.891 0 1.989 1.953 3.423 4.975 3.423 2.715 0 4.156-1.291 4.156-3.03 0-1.431-.933-2.186-3.078-3.703zm.793-5.308c.646-.634.702-1.51.702-2.008 0-1.976-1.198-5.053-3.519-5.053-.724 0-1.509.357-1.954.91-.473.572-.614 1.315-.614 2.029 0 1.84 1.091 4.891 3.49 4.891.7 0 1.453-.335 1.895-.769z" id="Fill-3" fill="#fff"/><path d="M86.858 43c-4.468 0-6.858-3.36-6.858-6.403 0-3.555 3.005-6.597 7.28-6.597 4.132 0 6.72 3.125 6.72 6.405 0 3.204-2.558 6.595-7.142 6.595zm3.494-2.198c.682-.871.847-1.959.847-3.019 0-2.398-1.183-6.971-4.681-6.971-.93 0-1.867.351-2.542.924-1.102.953-1.298 2.15-1.298 3.325 0 2.69 1.377 7.126 4.796 7.126 1.103 0 2.229-.516 2.878-1.385z" id="Fill-4" fill="#fff"/><path d="M72.858 43c-4.471 0-6.858-3.36-6.858-6.403 0-3.555 3.007-6.597 7.282-6.597 4.131 0 6.718 3.125 6.718 6.405 0 3.204-2.556 6.595-7.142 6.595zm3.496-2.198c.677-.871.846-1.959.846-3.019 0-2.398-1.187-6.971-4.684-6.971-.934 0-1.861.351-2.539.924-1.104.953-1.299 2.15-1.299 3.325 0 2.69 1.384 7.126 4.798 7.126 1.101 0 2.226-.516 2.877-1.385z" id="Fill-5" fill="#fff"/><path d="M64.011 42.711l-3.738.845c-1.515.236-2.875.443-4.315.443-7.21 0-9.958-5.242-9.958-9.345 0-5.01 3.893-9.655 10.559-9.655 1.41 0 2.767.205 4.003.538 1.959.544 2.876 1.214 3.448 1.606l-2.172 2.037-.916.202.653-1.027c-.884-.853-2.509-2.427-5.594-2.427-4.124 0-7.235 3.096-7.235 7.617 0 4.855 3.552 9.422 9.25 9.422 1.674 0 2.536-.332 3.318-.642v-4.162l-3.947.21 2.092-1.11h5.54l-.678.644c-.183.153-.207.206-.259.411-.027.235-.053.983-.053 1.245v3.148" id="Fill-6" fill="#fff"/><path d="M132.433 41.052v6.948h-1.433v-17.681h1.433v2.014c.933-1.302 2.639-2.333 4.62-2.333 3.553 0 5.947 2.596 5.947 6.705 0 4.082-2.393 6.732-5.947 6.732-1.87 0-3.546-.929-4.62-2.385zm9.098-4.241c0-3.13-1.683-5.517-4.666-5.517-1.853 0-3.625 1.412-4.395 2.643v5.719c.77 1.234 2.542 2.697 4.395 2.697 2.984 0 4.666-2.408 4.666-5.542z" id="Fill-7" fill="#fff"/><path fill="#fff" d="M144 25h1v18h-1z"/><path d="M161.31 46.541c.286.132.774.213 1.084.213.818 0 1.388-.345 1.899-1.565l.976-2.281-5.269-12.908h1.492l4.519 11.186 4.476-11.186h1.513l-6.426 15.617c-.667 1.609-1.716 2.383-3.157 2.383-.433 0-1.003-.079-1.337-.185l.23-1.274" fill="#fff"/><path d="M157.396 42.755c-.093-.433-.163-.814-.214-1.132-.042-.318-.067-.642-.067-.972-.477.677-1.153 1.24-2.017 1.684-.864.447-1.686.666-2.812.666-1.356 0-2.409-.322-3.16-.976-.749-.653-1.125-1.538-1.125-2.658 0-1.118.546-2.026 1.634-2.72 1.088-.697 2.494-1.044 4.234-1.044h3.247v-1.55c0-.893-.315-1.595-.944-2.104-.631-.51-1.517-.767-2.665-.767-1.05 0-1.901.236-2.549.694-.643.468-.966 1.048-.966 1.747h-1.425l-.029-.067c-.051-.942.395-1.769 1.343-2.483.944-.713 2.182-1.072 3.704-1.072 1.518 0 2.746.353 3.672 1.057.925.697 1.389 1.705 1.389 3.023v6.154c0 .442.022.869.079 1.285.059.417.153.833.277 1.236h-1.604zm-4.943-.895c1.182 0 2.031-.257 2.898-.764.862-.505 1.449-1.163 1.756-1.959v-2.408h-3.259c-1.245 0-2.276.268-3.087.808-.812.539-1.224 1.19-1.224 1.948 0 .715.261 1.287.779 1.72.52.438 1.23.655 2.137.655z" fill="#fff"/><path d="M46 12.995c0-2.431 2-3.995 4.487-3.995 1.667 0 2.757.718 3.436 1.598l-1.217.637c-.461-.579-1.27-1.03-2.219-1.03-1.692 0-2.948 1.157-2.948 2.79 0 1.609 1.257 2.79 2.948 2.79.872 0 1.641-.359 2.026-.694v-1.193h-2.564v-1.193h4.051v2.883c-.833.845-2.038 1.413-3.513 1.413-2.487 0-4.487-1.585-4.487-4.005" id="Fill-1" fill="#fff"/><path fill="#fff" d="M56 17v-8h6v1.236h-4.464v2.062h4.372v1.236h-4.372v2.231h4.464v1.236h-6"/><path fill="#fff" d="M65.714 17v-6.764h-2.714v-1.236h7v1.236h-2.714v6.764h-1.572"/><path id="Fill-4" fill="#fff" d="M75 9h1v8h-1z"/><path fill="#fff" d="M80.715 17v-6.764h-2.715v-1.236h7v1.236h-2.713v6.764h-1.572"/><path d="M90 13c0-2.308 1.861-4 4.5-4 2.625 0 4.5 1.693 4.5 4s-1.875 4-4.5 4c-2.64 0-4.5-1.693-4.5-4zm7.397-.059c0-1.583-1.14-2.765-2.897-2.765-1.77 0-2.898 1.181-2.898 2.765 0 1.571 1.128 2.765 2.898 2.765 1.757 0 2.897-1.193 2.897-2.765z" fill="#fff"/><path id="Fill-7" fill="#fff" d="M107.436 17l-4.817-5.721v5.721h-1.619v-8h1.661l4.719 5.542v-5.542h1.62v8h-1.564"/><g><path d="M25.412 27.295s-10.524-5.965-11.143-6.315c-.619-.35-1.235-.138-1.235.595v25.727c0 .548.44.786.978.482l11.4-6.462 2.214-1.254 3.562-2.018 5.553-3.147c.651-.369.588-.876.039-1.167-.55-.29-5.592-3.168-5.592-3.168l-5.776-3.273z" fill="url(#a)"/><g transform="translate(13 20) translate(0 .848)"><mask id="d"><use xlink:href="#b"/></mask><use fill="url(#c)" xlink:href="#b"/><path d="M12.393 6.445c-.02.048-10.541-5.928-11.154-6.233-.623-.397-1.239-.184-1.239.528v25.778c0 .566.44.805.929.528.585-.333 11.443-6.502 11.464-6.55l5.783-3.275c-.03.046 4.869-2.738 5.474-3.064.698-.413.635-.92.103-1.162-.567-.341-5.607-3.224-5.577-3.275l-5.783-3.275z" mask="url(#d)"/></g><g transform="translate(13 20) translate(.308 14.319)"><mask id="g"><use xlink:href="#e"/></mask><path d="M12.085-7.026c-.02.048-10.541-5.928-11.154-6.233-.623-.397-1.239-.184-1.239.528v25.778c0 .566.44.805.929.528.585-.333 11.443-6.502 11.464-6.55l5.783-3.275c-.03.046 4.869-2.738 5.474-3.064.698-.413.635-.92.103-1.162-.567-.341-5.607-3.224-5.577-3.275l-5.783-3.275z" fill="url(#f)" mask="url(#g)"/></g><g transform="translate(13 20) translate(.514 .848)"><mask id="j"><use xlink:href="#h"/></mask><use fill="url(#i)" xlink:href="#h"/><path d="M17.662 9.72l-5.783-3.275c-.02.048-10.541-5.928-11.154-6.233-.623-.397-1.239-.184-1.239.528v25.778c0 .566.44.805.929.528.585-.333 11.443-6.502 11.464-6.55l5.783-3.275c-.03.046 4.869-2.738 5.474-3.064.698-.413.635-.92.103-1.162-.567-.341-5.607-3.224-5.577-3.275" mask="url(#j)"/></g></g></g></svg> On 2017/11/20 17:06:07, Thomas Greiner wrote: > This image is quite outdated seeing that the colors are less saturated and the > Google logo is quite old when comparing it to the one from > https://play.google.com/intl/en_us/badges/ > > Let's also update the App Store logo to a newer versions in case we also went > with an old version for that one. Done. App Store button seems to be in line with the current guidelines. https://codereview.adblockplus.org/29592569/diff/29612630/updates.html File updates.html (right): https://codereview.adblockplus.org/29592569/diff/29612630/updates.html#newcode30 updates.html:30: <script src="updates.js"></script> On 2017/11/20 17:06:07, Thomas Greiner wrote: > This file is not included in the review so please make sure to add it. Done. https://codereview.adblockplus.org/29592569/diff/29612630/updates.html#newcode60 updates.html:60: <p id="optionsPageNews" class="i18n_updates_optionsPageNews"></p> On 2017/11/20 17:06:07, Thomas Greiner wrote: > Coding style: "Separate words in ID and class names by a hyphen." > > See https://adblockplus.org/en/coding-style#html-css and > https://google.github.io/styleguide/htmlcssguide.html#ID_and_Class_Name_Delim... Done. https://codereview.adblockplus.org/29592569/diff/29612630/updates.html#newcode75 updates.html:75: <a href="https://eyeo.to/adblockbrowser/ios/update-page" class="store-button applestore-button"> On 2017/11/20 17:06:07, Thomas Greiner wrote: > We use documentation links (see > https://bitbucket.org/adblockplus/spec/src/3a5d8435c857976db8e4b58d18148504f4...) > so we set any links using JavaScript. > > Looks like those links haven't been added to the spec yet though so we need to > make sure to add them before we roll out the update. > > Example: > https://hg.adblockplus.org/adblockplusui/file/tip/mobile-options.js#l435 I'm sort of confused as to how to use this. The `getDocLink` basically selects the actual <a> DOM element. But then we set the `href` attribute to the `link` argument. Here's where I'm lost. Where is this "link" arg defined? Can't I just select the element and set its href to "http://something.something"? Out of curiosity - why are these utility functions necessary?
https://codereview.adblockplus.org/29592569/diff/29612630/updates.html File updates.html (right): https://codereview.adblockplus.org/29592569/diff/29612630/updates.html#newcode75 updates.html:75: <a href="https://eyeo.to/adblockbrowser/ios/update-page" class="store-button applestore-button"> On 2017/11/26 17:08:39, martin wrote: > I'm sort of confused as to how to use this. The `getDocLink` basically selects > the actual <a> DOM element. But then we set the `href` attribute to the `link` > argument. Here's where I'm lost. Where is this "link" arg defined? No, the `getDocLink()` function gives you the URL for a given ID, not the `<a>` element. Example: getDocLink("id", (url) => { element.href = url; }); So for the ID "releases", `"id"` would be `"releases"`, `url` would be `https://adblockplus.org/redirect?link=releases&lang=en-US` and `element` would be the `<a>` element. > Can't I just > select the element and set its href to "http://something.something"? Out of > curiosity - why are these utility functions necessary? It's possible to do it like this but there are two benefits to this approach: 1) By using adblockplus.org/redirect we can ensure that we don't link to web pages that no longer exist. Unlike with websites, links that we include in the extension are there forever so linking to our own website allows us to dynamically change the link destination without having to release an extension update. 2) Centralizing the creation of this link using `getDocLink()` allows us to avoid code duplication. https://codereview.adblockplus.org/29592569/diff/29619567/locale/en_US/update... File locale/en_US/updates.json (right): https://codereview.adblockplus.org/29592569/diff/29619567/locale/en_US/update... locale/en_US/updates.json:1: { As mentioned on IRC, the texts in here differ quite a bit from those in the spec so let's update them. https://codereview.adblockplus.org/29592569/diff/29619567/skin/updates.css File skin/updates.css (right): https://codereview.adblockplus.org/29592569/diff/29619567/skin/updates.css#ne... skin/updates.css:78: cursor: pointer; Detail: Why is this necessary? `<a>` elements have this style by default and I don't see any other rules overriding it either. https://codereview.adblockplus.org/29592569/diff/29619567/skin/updates.css#ne... skin/updates.css:166: height: 50px; Suggestion: I noticed that the content jumps when the page loads in Chrome because the width of these images changes after they've been loaded. I'd assume that this is a rendering issue with Chrome and we could work around it by setting `min-width` and `min-height`. In Firefox it's even worse but the only solution there might be to wait until all strings have loaded before making the page visible so probably not worth the effort and risk. Therefore feel free to ignore this issue or add the mentioned workaround with a comment on top of it to explain why it's necessary. https://codereview.adblockplus.org/29592569/diff/29619567/skin/updates.css#ne... skin/updates.css:201: overflow: hidden; Detail: What's the purpose of this? There shouldn't be anything overflowing from what I see. https://codereview.adblockplus.org/29592569/diff/29619567/updates.html File updates.html (right): https://codereview.adblockplus.org/29592569/diff/29619567/updates.html#newcode75 updates.html:75: <a href="https://eyeo.to/adblockbrowser/ios/update-page" class="store-button applestore-button"> According to the spec those two links are supposed to open in a new tab. https://codereview.adblockplus.org/29592569/diff/29619567/updates.js File updates.js (right): https://codereview.adblockplus.org/29592569/diff/29619567/updates.js#newcode18 updates.js:18: /* globals setLinks, E */ Detail: You're not using `setLinks()` in this file so no need to list it here. https://codereview.adblockplus.org/29592569/diff/29619567/updates.js#newcode27 updates.js:27: const optionsPageLink = optionsPageLinkParent.getElementsByTagName("a")[0]; Detail: Using `document.querySelector()` you can directly select the element you want. e.g. document.querySelector("#options-page-news > a")
Submitted a new review. https://codereview.adblockplus.org/29592569/diff/29612630/updates.html File updates.html (right): https://codereview.adblockplus.org/29592569/diff/29612630/updates.html#newcode75 updates.html:75: <a href="https://eyeo.to/adblockbrowser/ios/update-page" class="store-button applestore-button"> On 2017/11/27 16:15:47, Thomas Greiner wrote: > On 2017/11/26 17:08:39, martin wrote: > > I'm sort of confused as to how to use this. The `getDocLink` basically selects > > the actual <a> DOM element. But then we set the `href` attribute to the `link` > > argument. Here's where I'm lost. Where is this "link" arg defined? > > No, the `getDocLink()` function gives you the URL for a given ID, not the `<a>` > element. > > Example: > getDocLink("id", (url) => > { > element.href = url; > }); > > So for the ID "releases", `"id"` would be `"releases"`, `url` would be > `https://adblockplus.org/redirect?link=releases&lang=en-US` and `element` would > be the `<a>` element. > > > Can't I just > > select the element and set its href to "http://something.something"? Out of > > curiosity - why are these utility functions necessary? > > It's possible to do it like this but there are two benefits to this approach: > > 1) By using adblockplus.org/redirect we can ensure that we don't link to web > pages that no longer exist. Unlike with websites, links that we include in the > extension are there forever so linking to our own website allows us to > dynamically change the link destination without having to release an extension > update. > > 2) Centralizing the creation of this link using `getDocLink()` allows us to > avoid code duplication. Aaaaah! I get it now. Thanks for the explanation. It makes sense now. https://codereview.adblockplus.org/29592569/diff/29619567/locale/en_US/update... File locale/en_US/updates.json (right): https://codereview.adblockplus.org/29592569/diff/29619567/locale/en_US/update... locale/en_US/updates.json:1: { On 2017/11/27 16:15:47, Thomas Greiner wrote: > As mentioned on IRC, the texts in here differ quite a bit from those in the spec > so let's update them. Done. https://codereview.adblockplus.org/29592569/diff/29619567/skin/updates.css File skin/updates.css (right): https://codereview.adblockplus.org/29592569/diff/29619567/skin/updates.css#ne... skin/updates.css:78: cursor: pointer; On 2017/11/27 16:15:48, Thomas Greiner wrote: > Detail: Why is this necessary? `<a>` elements have this style by default and I > don't see any other rules overriding it either. I *think* what's happening is that because the "Settings page" has no "href" the pointer reverts to "normal". I needed to include this rule to achieve the expected behaviour. Let me know if there's a different solution for that. https://codereview.adblockplus.org/29592569/diff/29619567/skin/updates.css#ne... skin/updates.css:166: height: 50px; On 2017/11/27 16:15:49, Thomas Greiner wrote: > Suggestion: I noticed that the content jumps when the page loads in Chrome > because the width of these images changes after they've been loaded. > I'd assume that this is a rendering issue with Chrome and we could work around > it by setting `min-width` and `min-height`. > > In Firefox it's even worse but the only solution there might be to wait until > all strings have loaded before making the page visible so probably not worth the > effort and risk. > I'm ignoring this issue for now. > Therefore feel free to ignore this issue or add the mentioned workaround with a > comment on top of it to explain why it's necessary. https://codereview.adblockplus.org/29592569/diff/29619567/skin/updates.css#ne... skin/updates.css:201: overflow: hidden; On 2017/11/27 16:15:49, Thomas Greiner wrote: > Detail: What's the purpose of this? There shouldn't be anything overflowing from > what I see. Oops. Was playing around with something and forgot to delete the rule. It's gone now. https://codereview.adblockplus.org/29592569/diff/29619567/updates.js File updates.js (right): https://codereview.adblockplus.org/29592569/diff/29619567/updates.js#newcode18 updates.js:18: /* globals setLinks, E */ On 2017/11/27 16:15:50, Thomas Greiner wrote: > Detail: You're not using `setLinks()` in this file so no need to list it here. I'm now using it to set the link for id="adblock-browser-text". https://codereview.adblockplus.org/29592569/diff/29619567/updates.js#newcode27 updates.js:27: const optionsPageLink = optionsPageLinkParent.getElementsByTagName("a")[0]; On 2017/11/27 16:15:51, Thomas Greiner wrote: > Detail: Using `document.querySelector()` you can directly select the element you > want. > > e.g. > document.querySelector("#options-page-news > a") Fixed.
https://codereview.adblockplus.org/29592569/diff/29619567/skin/updates.css File skin/updates.css (right): https://codereview.adblockplus.org/29592569/diff/29619567/skin/updates.css#ne... skin/updates.css:78: cursor: pointer; On 2017/12/05 14:37:31, martin wrote: > On 2017/11/27 16:15:48, Thomas Greiner wrote: > > Detail: Why is this necessary? `<a>` elements have this style by default and I > > don't see any other rules overriding it either. > > I *think* what's happening is that because the "Settings page" has no "href" > the pointer reverts to "normal". I needed to include this rule to achieve the > expected behaviour. Let me know if there's a different solution for that. Makes sense. Generally, we add `href="#"` for such links so you could either add that via JavaScript or add a comment here to explain why these styles are necessary. I just noticed, however, that you can also pass a function to `setLinks()` in which case it'll set the "href" attribute as well as the click listener automatically (see https://hg.adblockplus.org/adblockplusui/file/tip/common.js#l36). https://codereview.adblockplus.org/29592569/diff/29630703/locale/en_US/update... File locale/en_US/updates.json (right): https://codereview.adblockplus.org/29592569/diff/29630703/locale/en_US/update... locale/en_US/updates.json:12: "message": "Brand new <a>Settings page</a> - revamped and restyled - so it's easier to customize Adblock Plus just like you want it." The link in this text no longer exists from what I see in the spec. Instead, the text below should be hyperlinked but it's not being used anywhere yet from what I see. So I'd recommend appending the text "Check it out for yourself!" here and removing the "updates_optionsPageLink" string. https://codereview.adblockplus.org/29592569/diff/29630703/locale/en_US/update... locale/en_US/updates.json:24: "message": "If you like taking control on desktop, you can block ads and improve your privacy and security on mobile, too. Get the <a>Adblock Browser App</a> below." Detail: The link in the spec also includes "below". https://codereview.adblockplus.org/29592569/diff/29630703/updates.html File updates.html (right): https://codereview.adblockplus.org/29592569/diff/29630703/updates.html#newcode77 updates.html:77: <a id="adblock_browser_ios_store" class="store-button applestore-button" target="_blank"> Coding style: "Separate words in ID and class names by a hyphen." See https://google.github.io/styleguide/htmlcssguide.html#ID_and_Class_Name_Delim... https://codereview.adblockplus.org/29592569/diff/29630703/updates.js File updates.js (right): https://codereview.adblockplus.org/29592569/diff/29630703/updates.js#newcode44 updates.js:44: setLinks("adblock-browser-text", "https://adblockbrowser.org"); Again, let's not hardcode any links in the extension and instead use documentation links. https://codereview.adblockplus.org/29592569/diff/29630703/updates.js#newcode44 updates.js:44: setLinks("adblock-browser-text", "https://adblockbrowser.org"); The second argument is expected to be an array. See https://hg.adblockplus.org/adblockplusui/file/tip/common.js#l36
https://codereview.adblockplus.org/29592569/diff/29630703/updates.html File updates.html (right): https://codereview.adblockplus.org/29592569/diff/29630703/updates.html#newcode77 updates.html:77: <a id="adblock_browser_ios_store" class="store-button applestore-button" target="_blank"> On 2017/12/11 12:11:10, Thomas Greiner wrote: > Coding style: "Separate words in ID and class names by a hyphen." > > See > https://google.github.io/styleguide/htmlcssguide.html#ID_and_Class_Name_Delim... Can I change the `adblock_browser_ios_store` ID? I think this specific ID is what makes the doc-link work. https://codereview.adblockplus.org/29592569/diff/29630703/updates.js File updates.js (right): https://codereview.adblockplus.org/29592569/diff/29630703/updates.js#newcode44 updates.js:44: setLinks("adblock-browser-text", "https://adblockbrowser.org"); On 2017/12/11 12:11:11, Thomas Greiner wrote: > Again, let's not hardcode any links in the extension and instead use > documentation links. Couldn't find a doc-link to "https://adblockbrowser.org"... Is there a way to generate one or am I not looking in the right place?
https://codereview.adblockplus.org/29592569/diff/29630703/updates.html File updates.html (right): https://codereview.adblockplus.org/29592569/diff/29630703/updates.html#newcode77 updates.html:77: <a id="adblock_browser_ios_store" class="store-button applestore-button" target="_blank"> On 2017/12/14 11:25:16, martin wrote: > On 2017/12/11 12:11:10, Thomas Greiner wrote: > > Coding style: "Separate words in ID and class names by a hyphen." > > > > See > > > https://google.github.io/styleguide/htmlcssguide.html#ID_and_Class_Name_Delim... > > Can I change the `adblock_browser_ios_store` ID? I think this specific ID is > what makes the doc-link work. In the JavaScript file you're retrieving the URL for a doclink ID and assign it to an HTML element for its ID. So yes, those IDs are independent from each other and can therefore be different. https://codereview.adblockplus.org/29592569/diff/29630703/updates.js File updates.js (right): https://codereview.adblockplus.org/29592569/diff/29630703/updates.js#newcode44 updates.js:44: setLinks("adblock-browser-text", "https://adblockbrowser.org"); On 2017/12/14 11:25:16, martin wrote: > On 2017/12/11 12:11:11, Thomas Greiner wrote: > > Again, let's not hardcode any links in the extension and instead use > > documentation links. > > Couldn't find a doc-link to "https://adblockbrowser.org"... Is there a way to > generate one or am I not looking in the right place? The usual process is to update the spec and then inform Ops about implementing the redirect on the server-side. There may already be a process already in place to communicate this spec change to Ops so Winsley may be able to help you with that.
Published a new patch-set. I might have missed something - do let me know if you find something weird. https://codereview.adblockplus.org/29592569/diff/29619567/skin/updates.css File skin/updates.css (right): https://codereview.adblockplus.org/29592569/diff/29619567/skin/updates.css#ne... skin/updates.css:78: cursor: pointer; Added a comment here to explain the styles. On 2017/12/11 12:11:08, Thomas Greiner wrote: > On 2017/12/05 14:37:31, martin wrote: > > On 2017/11/27 16:15:48, Thomas Greiner wrote: > > > Detail: Why is this necessary? `<a>` elements have this style by default and > I > > > don't see any other rules overriding it either. > > > > I *think* what's happening is that because the "Settings page" has no "href" > > the pointer reverts to "normal". I needed to include this rule to achieve the > > expected behaviour. Let me know if there's a different solution for that. > > Makes sense. Generally, we add `href="#"` for such links so you could either add > that via JavaScript or add a comment here to explain why these styles are > necessary. > > I just noticed, however, that you can also pass a function to `setLinks()` in > which case it'll set the "href" attribute as well as the click listener > automatically (see > https://hg.adblockplus.org/adblockplusui/file/tip/common.js#l36). https://codereview.adblockplus.org/29592569/diff/29630703/locale/en_US/update... File locale/en_US/updates.json (right): https://codereview.adblockplus.org/29592569/diff/29630703/locale/en_US/update... locale/en_US/updates.json:12: "message": "Brand new <a>Settings page</a> - revamped and restyled - so it's easier to customize Adblock Plus just like you want it." On 2017/12/11 12:11:08, Thomas Greiner wrote: > The link in this text no longer exists from what I see in the spec. Instead, the > text below should be hyperlinked but it's not being used anywhere yet from what > I see. > > So I'd recommend appending the text "Check it out for yourself!" here and > removing the "updates_optionsPageLink" string. Done. https://codereview.adblockplus.org/29592569/diff/29630703/locale/en_US/update... locale/en_US/updates.json:24: "message": "If you like taking control on desktop, you can block ads and improve your privacy and security on mobile, too. Get the <a>Adblock Browser App</a> below." On 2017/12/11 12:11:10, Thomas Greiner wrote: > Detail: The link in the spec also includes "below". Done. https://codereview.adblockplus.org/29592569/diff/29630703/updates.html File updates.html (right): https://codereview.adblockplus.org/29592569/diff/29630703/updates.html#newcode77 updates.html:77: <a id="adblock_browser_ios_store" class="store-button applestore-button" target="_blank"> On 2017/12/11 12:11:10, Thomas Greiner wrote: > Coding style: "Separate words in ID and class names by a hyphen." > > See > https://google.github.io/styleguide/htmlcssguide.html#ID_and_Class_Name_Delim... Done. https://codereview.adblockplus.org/29592569/diff/29630703/updates.js File updates.js (right): https://codereview.adblockplus.org/29592569/diff/29630703/updates.js#newcode44 updates.js:44: setLinks("adblock-browser-text", "https://adblockbrowser.org"); On 2017/12/14 13:36:26, Thomas Greiner wrote: > On 2017/12/14 11:25:16, martin wrote: > > On 2017/12/11 12:11:11, Thomas Greiner wrote: > > > Again, let's not hardcode any links in the extension and instead use > > > documentation links. > > > > Couldn't find a doc-link to "https://adblockbrowser.org"... Is there a way to > > generate one or am I not looking in the right place? > > The usual process is to update the spec and then inform Ops about implementing > the redirect on the server-side. > > There may already be a process already in place to communicate this spec change > to Ops so Winsley may be able to help you with that. I `getDocLink`-ed this. It's currently not working, however Winsley knows about this and will make sure the doc-link is added at some future point.
Moving myself to CC.
Were checking the difference between requested translations and strings here, seems like some of the text do not reflect text in the specs. https://codereview.adblockplus.org/29592569/diff/29644588/locale/en_US/update... File locale/en_US/updates.json (right): https://codereview.adblockplus.org/29592569/diff/29644588/locale/en_US/update... locale/en_US/updates.json:3: "message": "Update Complete" Deatail: Would be great to do the capitalization in CSS, not to adapt the translations https://codereview.adblockplus.org/29592569/diff/29644588/locale/en_US/update... locale/en_US/updates.json:12: "message": "Brand new Settings page - revamped and restyled - so it's easier to customize Adblock Plus just like you want it. <a>Check it out for yourself</a>" The text is different in the Specs -> Brand new Settings page - revamped and restyled with more privacy and security options - so it's easier to [customize Adblock Plus just like you want it][1]. See -> https://gitlab.com/eyeo/spec/blob/updates_page/spec/abp/updates-page.md https://codereview.adblockplus.org/29592569/diff/29644588/locale/en_US/update... locale/en_US/updates.json:21: "message": "If you like taking control on desktop, you can block ads and improve your privacy and security on mobile, too. Get the <a>Adblock Browser App below</a>." The text is different in specs -> If you are taking control on desktop, you can block ads and improve your privacy and security on mobile too. Get the [Adblock Browser App below][1] https://codereview.adblockplus.org/29592569/diff/29644588/locale/en_US/update... locale/en_US/updates.json:23: } Question: Do we have current "New blocking technology! There are now more ways than ever to create new blocking filters that let you blast hidden ads on websites like Facebook, even if the Empire strikes back." text implemented ? See -> https://gitlab.com/eyeo/spec/blob/updates_page/spec/abp/updates-page.md#for-g...
https://codereview.adblockplus.org/29592569/diff/29644588/locale/en_US/update... File locale/en_US/updates.json (right): https://codereview.adblockplus.org/29592569/diff/29644588/locale/en_US/update... locale/en_US/updates.json:3: "message": "Update Complete" On 2018/01/05 14:01:47, saroyanm wrote: > Deatail: Would be great to do the capitalization in CSS, not to adapt the > translations Nevermind: Please make this consistent with the specs lower case "c" -> Update complete
The Adblock Browser website link doesn't work yet but other than that there's only a few small comments after which we should be good to go. https://codereview.adblockplus.org/29592569/diff/29630703/skin/updates.css File skin/updates.css (right): https://codereview.adblockplus.org/29592569/diff/29630703/skin/updates.css#ne... skin/updates.css:54: .graphic-column Detail: You can combine this rule with the one above to avoid duplication. https://codereview.adblockplus.org/29592569/diff/29630703/updates.html File updates.html (right): https://codereview.adblockplus.org/29592569/diff/29630703/updates.html#newcode64 updates.html:64: <img class="feature-icon" src="/skin/updates/icon-thumbs-up.svg" alt="thumbs-up-icon"/> Detail: Please remove the alt-text from all three feature icons. They're not translated and only describe purely decorative elements so we don't need to add it. You can leave them for the store buttons though since we decided not to translate them for this release. https://codereview.adblockplus.org/29592569/diff/29630703/updates.js File updates.js (right): https://codereview.adblockplus.org/29592569/diff/29630703/updates.js#newcode26 updates.js:26: const optionsPageLink = document.querySelector("#options-page-news > a"); Coding style: "Use const for constant expressions that could as well have been inlined (e.g. static parameters, imported modules, etc.)." See https://adblockplus.org/coding-style#javascript-modern A bit more information on that: By default we always use `let` and we only use `const` for actual constants and values imported from other modules. https://codereview.adblockplus.org/29592569/diff/29630703/updates.js#newcode44 updates.js:44: setLinks("adblock-browser-text", "https://adblockbrowser.org"); On 2017/12/19 12:59:52, martin wrote: > On 2017/12/14 13:36:26, Thomas Greiner wrote: > > On 2017/12/14 11:25:16, martin wrote: > > > On 2017/12/11 12:11:11, Thomas Greiner wrote: > > > > Again, let's not hardcode any links in the extension and instead use > > > > documentation links. > > > > > > Couldn't find a doc-link to "https://adblockbrowser.org"... Is there a way > to > > > generate one or am I not looking in the right place? > > > > The usual process is to update the spec and then inform Ops about implementing > > the redirect on the server-side. > > > > There may already be a process already in place to communicate this spec > change > > to Ops so Winsley may be able to help you with that. > > I `getDocLink`-ed this. It's currently not working, however Winsley knows about > this and will make sure the doc-link is added at some future point. Great, thanks. https://codereview.adblockplus.org/29592569/diff/29644588/skin/updates.css File skin/updates.css (right): https://codereview.adblockplus.org/29592569/diff/29644588/skin/updates.css#ne... skin/updates.css:76: /* Needed in order to achieve standard behavior for links, due to lacking href attribute */ Coding style: "Line length: 80 characters or less" See https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style... https://codereview.adblockplus.org/29592569/diff/29644588/updates.js File updates.js (right): https://codereview.adblockplus.org/29592569/diff/29644588/updates.js#newcode46 updates.js:46: E("adblock-browser-text").href = url; You're not retrieving the `<a>` element here because it's inside of the translated text. Therefore this link doesn't work. You can use `setLinks("adblock-browser-text", url)` to assign URLs to links within a translatable text.
Addressed the latest feedback and submitted a new patch-set. https://codereview.adblockplus.org/29592569/diff/29630703/skin/updates.css File skin/updates.css (right): https://codereview.adblockplus.org/29592569/diff/29630703/skin/updates.css#ne... skin/updates.css:54: .graphic-column On 2018/01/10 12:28:21, Thomas Greiner wrote: > Detail: You can combine this rule with the one above to avoid duplication. Done. https://codereview.adblockplus.org/29592569/diff/29630703/updates.html File updates.html (right): https://codereview.adblockplus.org/29592569/diff/29630703/updates.html#newcode64 updates.html:64: <img class="feature-icon" src="/skin/updates/icon-thumbs-up.svg" alt="thumbs-up-icon"/> On 2018/01/10 12:28:21, Thomas Greiner wrote: > Detail: Please remove the alt-text from all three feature icons. They're not > translated and only describe purely decorative elements so we don't need to add > it. > > You can leave them for the store buttons though since we decided not to > translate them for this release. Done. https://codereview.adblockplus.org/29592569/diff/29630703/updates.html#newcode77 updates.html:77: <a id="adblock_browser_ios_store" class="store-button applestore-button" target="_blank"> On 2017/12/11 12:11:10, Thomas Greiner wrote: > Coding style: "Separate words in ID and class names by a hyphen." > > See > https://google.github.io/styleguide/htmlcssguide.html#ID_and_Class_Name_Delim... Done. https://codereview.adblockplus.org/29592569/diff/29630703/updates.js File updates.js (right): https://codereview.adblockplus.org/29592569/diff/29630703/updates.js#newcode26 updates.js:26: const optionsPageLink = document.querySelector("#options-page-news > a"); On 2018/01/10 12:28:21, Thomas Greiner wrote: > Coding style: "Use const for constant expressions that could as well have been > inlined (e.g. static parameters, imported modules, etc.)." > > See https://adblockplus.org/coding-style#javascript-modern > > A bit more information on that: By default we always use `let` and we only use > `const` for actual constants and values imported from other modules. Done. https://codereview.adblockplus.org/29592569/diff/29644588/locale/en_US/update... File locale/en_US/updates.json (right): https://codereview.adblockplus.org/29592569/diff/29644588/locale/en_US/update... locale/en_US/updates.json:3: "message": "Update Complete" On 2018/01/05 14:01:47, saroyanm wrote: > Deatail: Would be great to do the capitalization in CSS, not to adapt the > translations Done. https://codereview.adblockplus.org/29592569/diff/29644588/locale/en_US/update... locale/en_US/updates.json:3: "message": "Update Complete" On 2018/01/05 14:09:09, saroyanm wrote: > On 2018/01/05 14:01:47, saroyanm wrote: > > Deatail: Would be great to do the capitalization in CSS, not to adapt the > > translations > > Nevermind: Please make this consistent with the specs lower case "c" -> Update > complete Done. https://codereview.adblockplus.org/29592569/diff/29644588/locale/en_US/update... locale/en_US/updates.json:12: "message": "Brand new Settings page - revamped and restyled - so it's easier to customize Adblock Plus just like you want it. <a>Check it out for yourself</a>" On 2018/01/05 14:01:48, saroyanm wrote: > The text is different in the Specs -> Brand new Settings page - revamped and > restyled with more privacy and security options - so it's easier to [customize > Adblock Plus just like you want it][1]. > > > See -> https://gitlab.com/eyeo/spec/blob/updates_page/spec/abp/updates-page.md Done. https://codereview.adblockplus.org/29592569/diff/29644588/locale/en_US/update... locale/en_US/updates.json:21: "message": "If you like taking control on desktop, you can block ads and improve your privacy and security on mobile, too. Get the <a>Adblock Browser App below</a>." On 2018/01/05 14:01:49, saroyanm wrote: > The text is different in specs -> If you are taking control on desktop, you can > block ads and improve your privacy and security on mobile too. Get the [Adblock > Browser App below][1] Done. https://codereview.adblockplus.org/29592569/diff/29644588/locale/en_US/update... locale/en_US/updates.json:23: } On 2018/01/05 14:01:47, saroyanm wrote: > Question: Do we have current "New blocking technology! There are now more ways > than ever to create new blocking filters that let you blast hidden ads on > websites like Facebook, even if the Empire strikes back." text implemented ? > > See -> > https://gitlab.com/eyeo/spec/blob/updates_page/spec/abp/updates-page.md#for-g... I'm not sure how this discussion ended and why we decided to drop these strings from here. Maybe Thomas can pitch in. https://codereview.adblockplus.org/29592569/diff/29644588/skin/updates.css File skin/updates.css (right): https://codereview.adblockplus.org/29592569/diff/29644588/skin/updates.css#ne... skin/updates.css:76: /* Needed in order to achieve standard behavior for links, due to lacking href attribute */ On 2018/01/10 12:28:23, Thomas Greiner wrote: > Coding style: "Line length: 80 characters or less" > > See > https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style... Done. https://codereview.adblockplus.org/29592569/diff/29644588/updates.js File updates.js (right): https://codereview.adblockplus.org/29592569/diff/29644588/updates.js#newcode46 updates.js:46: E("adblock-browser-text").href = url; On 2018/01/10 12:28:24, Thomas Greiner wrote: > You're not retrieving the `<a>` element here because it's inside of the > translated text. Therefore this link doesn't work. > > You can use `setLinks("adblock-browser-text", url)` to assign URLs to links > within a translatable text. Done.
LGTM with the coding style comment addressed https://codereview.adblockplus.org/29592569/diff/29664579/skin/updates.css File skin/updates.css (right): https://codereview.adblockplus.org/29592569/diff/29664579/skin/updates.css#ne... skin/updates.css:49: #graphic-column, .graphic-column Coding style: "Always start a new line for each selector and declaration." See https://google.github.io/styleguide/htmlcssguide.html#Selector_and_Declaratio...
One more thing: I noticed that the feature icons are larger than what's shown in the spec and some other dimensions seem to be off too. However, no need to block this review because of those small details so feel free to adapt the styles in a follow-up change. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
