Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(715)

Issue 29611624: Issue 6047 - Updated templates and uninstalled pages

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 weeks, 5 days ago by juliandoucette
Modified:
3 days, 1 hour ago
Reviewers:
ire
Base URL:
https://hg.adblockplus.org/web.adblockplus.org
Visibility:
Public.

Description

- Refactored default, minimal, and raw templates - Deprecated simple template - Updated uninstalled pages styles --- Please ignore defaults.css changes in this patchset. They have been provided here for the sake of convenience and should be reviewed separately here: - https://codereview.adblockplus.org/29612659/ - https://codereview.adblockplus.org/29612684/

Patch Set 1 #

Patch Set 2 : Fixed missing -1px in tablet media query #

Patch Set 3 : Updated button stytles #

Patch Set 4 : Changed document macro in html-attribute include, removed simple template, and removed temporary/unused resources include #

Total comments: 28

Patch Set 5 : Addressed comments #

Total comments: 13

Patch Set 6 : Addressed comments #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+581 lines, -632 lines) Patch
A includes/footer.tmpl View 1 chunk +69 lines, -0 lines 0 comments Download
A includes/html-attributes.tmpl View 1 2 3 4 5 1 chunk +6 lines, -0 lines 0 comments Download
A includes/meta/favicon.tmpl View 1 chunk +1 line, -0 lines 0 comments Download
A includes/meta/social.tmpl View 1 chunk +17 lines, -0 lines 0 comments Download
A includes/meta/standard.tmpl View 1 chunk +8 lines, -0 lines 0 comments Download
A includes/navbar.tmpl View 1 chunk +38 lines, -0 lines 0 comments Download
A includes/polyfills.tmpl View 1 chunk +14 lines, -0 lines 0 comments Download
A includes/scripts.tmpl View 1 chunk +1 line, -0 lines 0 comments Download
A includes/styles.tmpl View 1 chunk +2 lines, -0 lines 0 comments Download
A macros/pageitem.tmpl View 1 chunk +7 lines, -0 lines 0 comments Download
M pages/index.tmpl View 1 chunk +1 line, -1 line 0 comments Download
M pages/uninstalled.tmpl View 1 2 3 4 5 1 chunk +70 lines, -145 lines 0 comments Download
A pages/uninstalled-submit.html View 1 2 3 4 1 chunk +22 lines, -0 lines 1 comment Download
R pages/uninstalled-submit.md View 1 2 3 4 1 chunk +0 lines, -22 lines 0 comments Download
M static/css/defaults.css View 1 2 2 chunks +15 lines, -2 lines 0 comments Download
M static/css/main.css View 1 2 3 4 5 4 chunks +93 lines, -1 line 2 comments Download
R static/css/simple.css View 1 2 3 4 1 chunk +0 lines, -257 lines 0 comments Download
A static/css/uninstalled.css View 1 2 3 4 1 chunk +78 lines, -0 lines 0 comments Download
A static/js/uninstalled.js View 1 chunk +93 lines, -0 lines 0 comments Download
M templates/default.tmpl View 1 2 3 2 chunks +11 lines, -162 lines 0 comments Download
M templates/minimal.tmpl View 1 2 3 1 chunk +20 lines, -3 lines 0 comments Download
M templates/raw.tmpl View 1 2 3 1 chunk +15 lines, -1 line 0 comments Download
R templates/simple.tmpl View 1 2 3 1 chunk +0 lines, -38 lines 0 comments Download

Messages

Total messages: 13
juliandoucette
3 weeks, 5 days ago (2017-11-17 18:53:19 UTC) #1
juliandoucette
@Jeen @Paul: See https://drive.google.com/open?id=1B8rxkuGDGM9qh5M7SUccJPzmfJKkIz6q This change was meant to be an improvement, not a redesign. ...
3 weeks, 5 days ago (2017-11-17 19:04:17 UTC) #2
jeen
On 2017/11/17 19:04:17, juliandoucette wrote: > @Jeen @Paul: > > See https://drive.google.com/open?id=1B8rxkuGDGM9qh5M7SUccJPzmfJKkIz6q > > This ...
3 weeks, 3 days ago (2017-11-20 09:18:08 UTC) #3
juliandoucette
On 2017/11/20 09:18:08, jeen wrote: > @Julian, I like the overall design and layout of ...
3 weeks, 3 days ago (2017-11-20 15:55:47 UTC) #4
juliandoucette
On 2017/11/20 15:55:47, juliandoucette wrote: > I'm still waiting on feedback about the primary (red) ...
3 weeks, 1 day ago (2017-11-22 15:36:33 UTC) #5
ire
Thanks Julian! Here are my first impressions: https://codereview.adblockplus.org/29611624/diff/29618555/pages/uninstalled-submit.html File pages/uninstalled-submit.html (right): https://codereview.adblockplus.org/29611624/diff/29618555/pages/uninstalled-submit.html#newcode14 pages/uninstalled-submit.html:14: <aside id="reinstall" ...
2 weeks, 2 days ago (2017-11-27 19:48:22 UTC) #6
juliandoucette
Thanks Ire :) https://codereview.adblockplus.org/29611624/diff/29618555/pages/uninstalled-submit.html File pages/uninstalled-submit.html (right): https://codereview.adblockplus.org/29611624/diff/29618555/pages/uninstalled-submit.html#newcode14 pages/uninstalled-submit.html:14: <aside id="reinstall" class="bg-accent" > On 2017/11/27 ...
1 week, 3 days ago (2017-12-04 13:34:05 UTC) #7
ire
https://codereview.adblockplus.org/29611624/diff/29618555/pages/uninstalled-submit.html File pages/uninstalled-submit.html (right): https://codereview.adblockplus.org/29611624/diff/29618555/pages/uninstalled-submit.html#newcode18 pages/uninstalled-submit.html:18: <a href="index" class="button"> On 2017/12/04 13:34:04, juliandoucette wrote: > ...
1 week, 2 days ago (2017-12-05 10:28:01 UTC) #8
juliandoucette
Thanks Ire, I will update and separate this a bit. https://codereview.adblockplus.org/29611624/diff/29618555/pages/uninstalled-submit.html File pages/uninstalled-submit.html (right): https://codereview.adblockplus.org/29611624/diff/29618555/pages/uninstalled-submit.html#newcode18 ...
1 week, 1 day ago (2017-12-06 16:05:07 UTC) #9
juliandoucette
Updated. https://codereview.adblockplus.org/29611624/diff/29618555/pages/uninstalled-submit.html File pages/uninstalled-submit.html (right): https://codereview.adblockplus.org/29611624/diff/29618555/pages/uninstalled-submit.html#newcode17 pages/uninstalled-submit.html:17: {{ reinstall-headline[Text next to the Reinstallation button] Did ...
1 week, 1 day ago (2017-12-06 16:30:54 UTC) #10
ire
Thanks Julian! https://codereview.adblockplus.org/29611624/diff/29618555/pages/uninstalled-submit.html File pages/uninstalled-submit.html (right): https://codereview.adblockplus.org/29611624/diff/29618555/pages/uninstalled-submit.html#newcode14 pages/uninstalled-submit.html:14: <aside id="reinstall" class="bg-accent" > On 2017/12/04 13:34:04, ...
1 week ago (2017-12-07 12:36:27 UTC) #11
juliandoucette
Updated. https://codereview.adblockplus.org/29611624/diff/29631675/includes/html-attributes.tmpl File includes/html-attributes.tmpl (right): https://codereview.adblockplus.org/29611624/diff/29631675/includes/html-attributes.tmpl#newcode1 includes/html-attributes.tmpl:1: lang="{{ locale }}" On 2017/12/07 12:36:26, ire wrote: ...
6 days ago (2017-12-08 16:49:53 UTC) #12
ire
3 days, 1 hour ago (2017-12-11 15:53:49 UTC) #13
I think you missed these comments:

On 2017/12/07 12:36:27, ire wrote:
> Thanks Julian!
> 
>
https://codereview.adblockplus.org/29611624/diff/29618555/pages/uninstalled-s...
> File pages/uninstalled-submit.html (right):
> 
>
https://codereview.adblockplus.org/29611624/diff/29618555/pages/uninstalled-s...
> pages/uninstalled-submit.html:14: <aside id="reinstall" class="bg-accent" >
> On 2017/12/04 13:34:04, juliandoucette wrote:
> > On 2017/11/27 19:48:20, ire wrote:
> > > Suggest: Since this is used on multiple pages, make it an include
> > 
> > Agreed.
> 
> You haven't done this yet.
> 
>
https://codereview.adblockplus.org/29611624/diff/29618555/pages/uninstalled.tmpl
> File pages/uninstalled.tmpl (right):
> 
>
https://codereview.adblockplus.org/29611624/diff/29618555/pages/uninstalled.t...
> pages/uninstalled.tmpl:51: <textarea
> On 2017/11/27 19:48:21, ire wrote:
> > The textarea has a fixed width so doesn't fit the screen on mobile
> 
> This hasn't been fixed yet.


-----

https://codereview.adblockplus.org/29611624/diff/29631675/includes/html-attri...
File includes/html-attributes.tmpl (right):

https://codereview.adblockplus.org/29611624/diff/29631675/includes/html-attri...
includes/html-attributes.tmpl:1: lang="{{ locale }}"
On 2017/12/08 16:49:51, juliandoucette wrote:
> On 2017/12/07 12:36:26, ire wrote:
> > This include doesn't seem to be working. None of these attributes are
applying
> 
> Apparently this was mistaken for page meta data :D
> 
> I'll file a bug and add a space to make this work for now.

Ah! Ack.

https://codereview.adblockplus.org/29611624/diff/29631675/static/css/main.css
File static/css/main.css (right):

https://codereview.adblockplus.org/29611624/diff/29631675/static/css/main.css...
static/css/main.css:241: .bg-accent .button.secondary
On 2017/12/08 16:49:52, juliandoucette wrote:
> On 2017/12/07 12:36:27, ire wrote:
> > NIT: I think this is a style of button that could exist outside of the
> > .bg-accent. I think it makes sense to have this be it's own style of button,
> > e.g. .button.secondary-o , instead of a .button.secondary that is within a
> > .bg-accent. 
> 
> What about .button.inverse?

That works now, but what if we wanted to have an "inverse" button that uses the
primary colours instead? 

(This is a NIT, if we ever do come across this situation we can handle it then)

https://codereview.adblockplus.org/29611624/diff/29631675/static/css/uninstal...
File static/css/uninstalled.css (right):

https://codereview.adblockplus.org/29611624/diff/29631675/static/css/uninstal...
static/css/uninstalled.css:11: margin: 1em 0;
On 2017/12/08 16:49:52, juliandoucette wrote:
> On 2017/12/07 12:36:27, ire wrote:
> > I don't think this is doing anything
> 
> It adds space above/below the button on mobile. It's ~necessary because the
> button is inline on desktop and block on mobile.

Acknowledged.

https://codereview.adblockplus.org/29611624/diff/29633758/pages/uninstalled-s...
File pages/uninstalled-submit.html (right):

https://codereview.adblockplus.org/29611624/diff/29633758/pages/uninstalled-s...
pages/uninstalled-submit.html:19: {{ reinstall[Reinstall button text] Reinstall
Now }}
This button doesn't look like the one on the uninstalled page. The text is red
and there is no border.

https://codereview.adblockplus.org/29611624/diff/29633758/static/css/main.css
File static/css/main.css (right):

https://codereview.adblockplus.org/29611624/diff/29633758/static/css/main.css...
static/css/main.css:242: a.button.inverse
On 2017/12/08 16:49:52, juliandoucette wrote:
> necessary to be more specific than `.content a` :/

Acknowledged.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5