updates.html - Issue 29592569: Issue 5943 - Implement Updates Page for Adblock Plus extension

Side by Side Diff: updates.html

Issue 29592569: Issue 5943 - Implement Updates Page for Adblock Plus extension (Closed) Base URL: https://hg.adblockplus.org/adblockplusui/

Patch Set: Created Oct. 30, 2017, 10:37 a.m.

Left:
Right:

Use n/p to move between diff chunks; N/P to move between comments.

Jump to:

View unified diff | Download patch

OLD	NEW
(Empty)
	1 <!DOCTYPE html>

	2 <!--

	3 - This file is part of Adblock Plus <https://adblockplus.org/>,
	juliandoucette 2017/10/30 15:30:11 This file isn't part of adblockplus.org This file isn't part of adblockplus.org martin 2017/11/03 10:02:15 Hmm it looks like this comment block is in every . Show quoted text 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? juliandoucette 2017/11/03 11:06:05 I was mistaken. Show quoted text 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.
	4 - Copyright (C) 2006-present eyeo GmbH

	5 -

	6 - Adblock Plus is free software: you can redistribute it and/or modify

	7 - it under the terms of the GNU General Public License version 3 as

	8 - published by the Free Software Foundation.

	9 -

	10 - Adblock Plus is distributed in the hope that it will be useful,

	11 - but WITHOUT ANY WARRANTY; without even the implied warranty of

	12 - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the

	13 - GNU General Public License for more details.

	14 -

	15 - You should have received a copy of the GNU General Public License

	16 - along with Adblock Plus. If not, see <http://www.gnu.org/licenses/>.

	17 -->

	18

	19 <html>

	20 <head>

	21 <title class="i18n_firstRun_title"></title>

	22 <meta charset="utf-8" />

	23 <meta name="viewport" content="width=device-width, initial-scale=1" />

	24 <link type="text/css" href="skin/common.css" rel="stylesheet" />

	25 <link type="text/css" href="skin/defaults.css" rel="stylesheet" />
	juliandoucette 2017/10/30 15:30:11 You didn't upload default.css with this review or You didn't upload default.css with this review or mention that this review should apply on top of another. martin 2017/11/03 10:02:14 I actually removed the default.css dependency as I Show quoted text 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.
	26 <link type="text/css" href="skin/updates.css" rel="stylesheet"/>

	27 <script type="text/javascript" src="polyfill.js"></script>

	28 <script type="text/javascript" src="ext/common.js"></script>

	29 <script type="text/javascript" src="ext/content.js"></script>

	30 <script type="text/javascript" src="common.js"></script>

	31 <script type="text/javascript" src="i18n.js"></script>

	32 </head>

	33

	34 <body>

	35

	36 <main>
	juliandoucette 2017/10/30 15:30:11 <main> has no heading. I suggest that you use the <main> has no heading. I suggest that you use the first <section> as a <header> instead to fix this. martin 2017/11/03 10:02:14 First <section> is now a <header> element. Show quoted text 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.
	37

	38 <div class="container content full-width-container">
	juliandoucette 2017/10/30 15:30:11 .container and .full-width-container are contradic .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. martin 2017/11/03 10:02:14 Ended up simply creating an id called "container". Show quoted text 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".
	39

	40 <div class="row">

	41

	42 <section class="column graphic-column">

	43 <div class="update-graphic-container">

	44 <div class="update-graphic-content">

	45 <img src="/skin/updates-page/abp-logo.svg" alt="adbl ock-plus-logo"/>

	46 <div class="version-details">

	47 <h2>Update Complete</h2>

	48 <span>v1.13.4</span>

	49 </div>

	50

	51 </div>

	52 </section>

	53

	54 <section class="column content-column">

	55

	56 <div class="content-column-entries">

	57

	58 <hgroup>

	59 <h1>Adblock Plus has been updated!</h1>

	60 <h2>What's new?</h2>
	juliandoucette 2017/10/30 15:30:11 NIT: It's recommended to use header instead of hgr 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/ martin 2017/11/03 10:02:13 Oh I've done this so many times, didn't realise it Show quoted text 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!
	61 </hgroup>

	62

	63 <div class="feature-entry">
	juliandoucette 2017/10/30 15:30:10 Suggest: I think these could be <article>s Suggest: I think these could be <article>s martin 2017/11/03 10:02:13 feature-entries are now <article>s Show quoted text On 2017/10/30 15:30:10, juliandoucette wrote: > Suggest: I think these could be <article>s feature-entries are now <article>s
	64 <img class="feature-icon" src="/skin/updates-page/ic on-rocket.svg" alt="rocket-icon"/>

	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>
	juliandoucette 2017/10/30 15:30:10 NIT: I think the first text could be an inline hea 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. martin 2017/11/03 10:02:14 I totally get what you mean, however I don't want Show quoted text 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. juliandoucette 2017/11/03 11:06:05 Ack. I think that these should only be <article>s Show quoted text 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).
	66 </div>

	67

	68 <div class="feature-entry">

	69 <img class="feature-icon" src="/skin/updates-page/ic on-thumbs-up.svg" alt="thumbs-up-icon"/>

	70 <p>Block ads on Facebook again! New blocking tech al lows for fast fixes in case <strike>the Empire</strike> Facebook strikes back.</ p>

	71 </div>

	72

	73 <div class="custom-feature-entry">

	74 <h2>But what about your phone?</h2>

	75 <div class="feature-entry">

	76 <img class="feature-icon" src="/skin/updates-pag e/icon-mobile.svg" alt="thumbs-up-icon"/>
	juliandoucette 2017/10/30 15:30:10 NIT: The phone alignment looks weird. Maybe align 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? martin 2017/11/03 10:02:13 It doesn't look that bad to me TBH. Every other ic Show quoted text 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. juliandoucette 2017/11/03 11:06:06 Acknowledged. Looks fine to me on second glance : Show quoted text 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 :/
	77 <p>If you like taking control on desktop, you ca n block ads and improve your privacy and security on mobile, too. Get the Adbloc k Browser App below.</p>

	78 </div>

	79 <div class="store-buttons">

	80 <a href="" class="store-button applestore-button ">

	81 <img src="skin/updates-page/appstore-bg.svg" alt="apple store button">

	82 </a>

	83 <a href="" class="store-button googleplay-button ">

	84 <img src="skin/updates-page/googleplay-bg.sv g" alt="google play store button">

	85 </a>

	86 </div>

	87 </div>

	88

	89 </div>

	90

	91 </section>

	92

	93 </div>

	94

	95 </div>

	96

	97 </main>

	98

	99 </body>

	100

	101 </html>

OLD	NEW

« skin/updates.css ('K') | « skin/updates-page/icon-thumbs-up.svg ('k') | no next file » | no next file with comments »