Issue 29551738: Issue 5634 - Replaced logo and refactored navbar width and colors (Closed)
|
Created:
Sept. 22, 2017, 1:13 a.m. by juliandoucette Modified:
Oct. 11, 2017, 12:04 p.m. Reviewers:
ire Base URL:
https://hg.adblockplus.org/web.adblockplus.org Visibility:
Public. |
Patch Set 1 #
Total comments: 23
Patch Set 2 : Rebased #Patch Set 3 : Minor refactoring, added fonts, added install background-color #Patch Set 4 : Major refactoring & mobile styles #
Total comments: 15
Patch Set 5 : Fixed .container #Patch Set 6 : Fixed navbar-logo-2x #
Total comments: 27
Patch Set 7 : Rebased #Patch Set 8 : Refactored implementation #
Total comments: 7
Patch Set 9 : Removed merge artifact #Patch Set 10 : Rebased #
Total comments: 44
Patch Set 11 : Detailed refactor after rebase (see comments) #Patch Set 12 : Moved irrelivant IE CSS from main.css to main-desktop.css where it was originally #
Total comments: 12
Patch Set 13 : Addressed comments #
MessagesTotal messages: 27
(I haven't figured out how to break this down into smaller chunks yet.)
https://codereview.adblockplus.org/29551738/diff/29551739/static/css/main.css File static/css/main.css (left): https://codereview.adblockplus.org/29551738/diff/29551739/static/css/main.css... static/css/main.css:110: .locale-code Note: I figured out that I could use the builtin upper filter to capitalize these codes instead. I will update my previous patchset later.
Thanks. Initial comments below. Sorry if there's anything I commented on that you already planed to handle in a separate issue :) > (I haven't figured out how to break this down into smaller chunks yet.) Ack. https://codereview.adblockplus.org/29551738/diff/29551739/static/css/main.css File static/css/main.css (left): https://codereview.adblockplus.org/29551738/diff/29551739/static/css/main.css... static/css/main.css:110: .locale-code On 2017/09/22 01:21:14, juliandoucette wrote: > Note: I figured out that I could use the builtin upper filter to capitalize > these codes instead. I will update my previous patchset later. Acknowledged. https://codereview.adblockplus.org/29551738/diff/29551739/templates/default.tmpl File templates/default.tmpl (left): https://codereview.adblockplus.org/29551738/diff/29551739/templates/default.t... templates/default.tmpl:91: <header> This opening <header> element is missing https://codereview.adblockplus.org/29551738/diff/29551739/templates/default.tmpl File templates/default.tmpl (right): https://codereview.adblockplus.org/29551738/diff/29551739/templates/default.t... templates/default.tmpl:85: <li class="selected"><a>{{get_string(name, "menu")}}</a></li> Why doesn't this have an href attribute? https://codereview.adblockplus.org/29551738/diff/29551739/templates/default.t... templates/default.tmpl:95: alt="Adblock Plus" Missing translation (also for title) https://codereview.adblockplus.org/29551738/diff/29551739/templates/default.t... templates/default.tmpl:98: srcset="/img/header-logo-2x.png"> Missing 2x https://codereview.adblockplus.org/29551738/diff/29551739/templates/default.t... templates/default.tmpl:102: <li class="selected first"><a>{{get_string("installation", "menu")}}</a></li> Why doesn't this have an href attribute? https://codereview.adblockplus.org/29551738/diff/29551739/templates/default.t... templates/default.tmpl:104: <li class="install-link first">{{"index"|linkify}}{{get_string("installation", "menu")}} <span class="sprite install-link-icon"></span></a></li> The install link comes last in the design https://codereview.adblockplus.org/29551738/diff/29551739/templates/default.t... templates/default.tmpl:104: <li class="install-link first">{{"index"|linkify}}{{get_string("installation", "menu")}} <span class="sprite install-link-icon"></span></a></li> Why doesn't it have a black background-color like the design? https://codereview.adblockplus.org/29551738/diff/29551739/templates/default.t... templates/default.tmpl:110: <a href="#" id="current-language" class="dropdown-toggle"> Shouldn't this be a button?
https://codereview.adblockplus.org/29551738/diff/29551739/templates/default.tmpl File templates/default.tmpl (right): https://codereview.adblockplus.org/29551738/diff/29551739/templates/default.t... templates/default.tmpl:85: <li class="selected"><a>{{get_string(name, "menu")}}</a></li> On 2017/09/22 09:30:00, ire wrote: > Why doesn't this have an href attribute? No good reason. I just did the minimum I needed to. I will add href="#". https://codereview.adblockplus.org/29551738/diff/29551739/templates/default.t... templates/default.tmpl:95: alt="Adblock Plus" On 2017/09/22 09:29:59, ire wrote: > Missing translation (also for title) We don't translate our brand name. I should probably use a translation string anyway. Thanks for pointing this out. https://codereview.adblockplus.org/29551738/diff/29551739/templates/default.t... templates/default.tmpl:98: srcset="/img/header-logo-2x.png"> On 2017/09/22 09:29:58, ire wrote: > Missing 2x Acknowledged. https://codereview.adblockplus.org/29551738/diff/29551739/templates/default.t... templates/default.tmpl:102: <li class="selected first"><a>{{get_string("installation", "menu")}}</a></li> On 2017/09/22 09:30:00, ire wrote: > Why doesn't this have an href attribute? Same as above. https://codereview.adblockplus.org/29551738/diff/29551739/templates/default.t... templates/default.tmpl:104: <li class="install-link first">{{"index"|linkify}}{{get_string("installation", "menu")}} <span class="sprite install-link-icon"></span></a></li> On 2017/09/22 09:29:59, ire wrote: > The install link comes last in the design This change is design only; not content. https://codereview.adblockplus.org/29551738/diff/29551739/templates/default.t... templates/default.tmpl:104: <li class="install-link first">{{"index"|linkify}}{{get_string("installation", "menu")}} <span class="sprite install-link-icon"></span></a></li> On 2017/09/22 09:29:59, ire wrote: > Why doesn't it have a black background-color like the design? Same as above. https://codereview.adblockplus.org/29551738/diff/29551739/templates/default.t... templates/default.tmpl:110: <a href="#" id="current-language" class="dropdown-toggle"> On 2017/09/22 09:29:59, ire wrote: > Shouldn't this be a button? Yes. But I'll refactor this in https://issues.adblockplus.org/ticket/5753.
This now applies on top of https://bitbucket.org/adblockplus/adblockplus.org
https://codereview.adblockplus.org/29551738/diff/29551739/templates/default.tmpl File templates/default.tmpl (right): https://codereview.adblockplus.org/29551738/diff/29551739/templates/default.t... templates/default.tmpl:85: <li class="selected"><a>{{get_string(name, "menu")}}</a></li> On 2017/09/22 09:30:00, ire wrote: > Why doesn't this have an href attribute? Done. https://codereview.adblockplus.org/29551738/diff/29551739/templates/default.t... templates/default.tmpl:95: alt="Adblock Plus" On 2017/09/22 09:29:59, ire wrote: > Missing translation (also for title) Done. https://codereview.adblockplus.org/29551738/diff/29551739/templates/default.t... templates/default.tmpl:98: srcset="/img/header-logo-2x.png"> On 2017/09/22 09:29:58, ire wrote: > Missing 2x Done. https://codereview.adblockplus.org/29551738/diff/29551739/templates/default.t... templates/default.tmpl:102: <li class="selected first"><a>{{get_string("installation", "menu")}}</a></li> On 2017/09/22 09:30:00, ire wrote: > Why doesn't this have an href attribute? Done. https://codereview.adblockplus.org/29551738/diff/29551739/templates/default.t... templates/default.tmpl:104: <li class="install-link first">{{"index"|linkify}}{{get_string("installation", "menu")}} <span class="sprite install-link-icon"></span></a></li> On 2017/09/22 09:29:59, ire wrote: > Why doesn't it have a black background-color like the design? Done.
- High priority - Ready for review - Added notes I tried not to overdo this... But it wasn't easy. The existing implementation needs a lot of work IMO. https://codereview.adblockplus.org/29551738/diff/29557781/static/css/main.css File static/css/main.css (left): https://codereview.adblockplus.org/29551738/diff/29557781/static/css/main.css... static/css/main.css:108: #flag-ar Note: I probably didn't need to remove these in this issue. I may have forgotten artifacts of these flags elsewhere. https://codereview.adblockplus.org/29551738/diff/29557781/static/css/main.css File static/css/main.css (right): https://codereview.adblockplus.org/29551738/diff/29557781/static/css/main.css... static/css/main.css:2: /* cyrillic-ext */ Note: This is temporary for this review only because I couldn't find a download link for this web font in these formats online. https://codereview.adblockplus.org/29551738/diff/29557781/static/css/main.css... static/css/main.css:187: @media (min-width: 1200px) Note: 1200 = 1170 + 30... This is an artifact left-over from when I applied margin/padding differently. It should be 1170. https://codereview.adblockplus.org/29551738/diff/29557781/static/css/main.css... static/css/main.css:187: @media (min-width: 1200px) Done. https://codereview.adblockplus.org/29551738/diff/29557781/templates/default.tmpl File templates/default.tmpl (right): https://codereview.adblockplus.org/29551738/diff/29557781/templates/default.t... templates/default.tmpl:79: {% macro pageitem(pagename) %} Note: I didn't really need to change this. I do think, off the top of my head, that removing the <a> from a selected <li> is the wrong approach. But I didn't really think about it. I'll undo this change if it's not obvious that I'm right above (off the top of my head).
Fixed a few issues (see Patchset titles). I had a hard time with this today. I'll review with fresh eyes tomorrow. https://codereview.adblockplus.org/29551738/diff/29557781/templates/default.tmpl File templates/default.tmpl (right): https://codereview.adblockplus.org/29551738/diff/29557781/templates/default.t... templates/default.tmpl:91: srcset="/img/navbar-logo.svg 2x"> Missing "-2x" https://codereview.adblockplus.org/29551738/diff/29557781/templates/default.t... templates/default.tmpl:91: srcset="/img/navbar-logo.svg 2x"> Done.
Speaking of SVG (before I forget) - Our CMS seems to serve SVG with the
incorrect mimetype. I'm not sure that you will run into this issue because you
said that you didn't (and I did) developing the help center.
Anyway, if you do encounter this issue, you can apply the following patch to fix
the CMS temporarily:
diff --git a/cms/bin/test_server.py b/cms/bin/test_server.py
--- a/cms/bin/test_server.py
+++ b/cms/bin/test_server.py
@@ -108,16 +108,18 @@
path = environ.get('PATH_INFO')
data = get_data(path)
if data is None:
return show_error(start_response, '404 Not Found', uri=path)
mime = mimetypes.guess_type(path)[0] or 'text/html'
+ if path.endswith('.svg'): mime = 'image/svg+xml'
+
if isinstance(data, unicode):
data = data.encode(UNICODE_ENCODING)
mime = '%s; charset=%s' % (mime, UNICODE_ENCODING)
start_response('200 OK', [('Content-Type', mime)])
return [data]
if __name__ == '__main__':
Also (before I forget)
- Jeen asked me to remove the install link
- I know that I didn't implement the collapsed menu on small screens as
specified in the design
- I plan to address that separately
- In the meantime, I tried to keep it close to how it looks today
On 2017/09/28 01:09:59, juliandoucette wrote:
> Speaking of SVG (before I forget) - Our CMS seems to serve SVG with the
> incorrect mimetype. I'm not sure that you will run into this issue because you
> said that you didn't (and I did) developing the help center.
>
> Anyway, if you do encounter this issue, you can apply the following patch to
fix
> the CMS temporarily:
>
> diff --git a/cms/bin/test_server.py b/cms/bin/test_server.py
> --- a/cms/bin/test_server.py
> +++ b/cms/bin/test_server.py
> @@ -108,16 +108,18 @@
> path = environ.get('PATH_INFO')
>
> data = get_data(path)
> if data is None:
> return show_error(start_response, '404 Not Found', uri=path)
>
> mime = mimetypes.guess_type(path)[0] or 'text/html'
>
> + if path.endswith('.svg'): mime = 'image/svg+xml'
> +
> if isinstance(data, unicode):
> data = data.encode(UNICODE_ENCODING)
> mime = '%s; charset=%s' % (mime, UNICODE_ENCODING)
>
> start_response('200 OK', [('Content-Type', mime)])
> return [data]
>
> if __name__ == '__main__':
Thanks! I didn't get any issues :)
On 2017/09/28 01:14:58, juliandoucette wrote: > Also (before I forget) > > - Jeen asked me to remove the install link Ack. > - I know that I didn't implement the collapsed menu on small screens as > specified in the design > - I plan to address that separately > - In the meantime, I tried to keep it close to how it looks today Ack. Makes sense.
Thanks Julian! Comments below. https://codereview.adblockplus.org/29551738/diff/29551739/templates/default.tmpl File templates/default.tmpl (right): https://codereview.adblockplus.org/29551738/diff/29551739/templates/default.t... templates/default.tmpl:110: <a href="#" id="current-language" class="dropdown-toggle"> On 2017/09/22 11:50:42, juliandoucette wrote: > On 2017/09/22 09:29:59, ire wrote: > > Shouldn't this be a button? > > Yes. But I'll refactor this in https://issues.adblockplus.org/ticket/5753. Acknowledged. https://codereview.adblockplus.org/29551738/diff/29557781/static/css/main.css File static/css/main.css (left): https://codereview.adblockplus.org/29551738/diff/29557781/static/css/main.css... static/css/main.css:108: #flag-ar On 2017/09/28 01:00:27, juliandoucette wrote: > Note: I probably didn't need to remove these in this issue. I may have forgotten > artifacts of these flags elsewhere. Ack. Do you want to keep them and remove the flags completely in a separate issue? I don't mind either way https://codereview.adblockplus.org/29551738/diff/29557781/static/css/main.css File static/css/main.css (right): https://codereview.adblockplus.org/29551738/diff/29557781/static/css/main.css... static/css/main.css:2: /* cyrillic-ext */ On 2017/09/28 01:00:28, juliandoucette wrote: > Note: This is temporary for this review only because I couldn't find a download > link for this web font in these formats online. Acknowledged. https://codereview.adblockplus.org/29551738/diff/29557781/static/css/main.css... static/css/main.css:117: font-family: 'Source Sans Pro', sans-serif; Note (for another issue probably): I think we should use `font-display: swap` on the @font-faces and provide a system font fallback for our font-families. https://developer.mozilla.org/en-US/docs/Web/CSS/@font-face/font-display Even though support is low, it's doesn't interfere with non-supporting browsers and is a good performance improvement for those that do. https://codereview.adblockplus.org/29551738/diff/29557781/static/css/main.css... static/css/main.css:117: font-family: 'Source Sans Pro', sans-serif; NIT: Double quotation marks (also elsewhere in the file) https://codereview.adblockplus.org/29551738/diff/29557781/static/css/main.css... static/css/main.css:187: @media (min-width: 1200px) On 2017/09/28 01:00:28, juliandoucette wrote: > Note: 1200 = 1170 + 30... This is an artifact left-over from when I applied > margin/padding differently. It should be 1170. Acknowledged. https://codereview.adblockplus.org/29551738/diff/29557781/static/css/main.css... static/css/main.css:427: #selected-locale::after :after instead of ::after for IE8 https://codereview.adblockplus.org/29551738/diff/29557781/static/css/main.css... static/css/main.css:432: margin-left: .255em; NIT: "Don't omit the optional leading 0 for decimal numbers." https://codereview.adblockplus.org/29551738/diff/29557781/templates/default.tmpl File templates/default.tmpl (right): https://codereview.adblockplus.org/29551738/diff/29557781/templates/default.t... templates/default.tmpl:79: {% macro pageitem(pagename) %} On 2017/09/28 01:00:28, juliandoucette wrote: > Note: I didn't really need to change this. I do think, off the top of my head, > that removing the <a> from a selected <li> is the wrong approach. But I didn't > really think about it. I'll undo this change if it's not obvious that I'm right > above (off the top of my head). I think you're right. The link should remain even if the page is currently selected. https://codereview.adblockplus.org/29551738/diff/29557801/static/css/main.css File static/css/main.css (right): https://codereview.adblockplus.org/29551738/diff/29557801/static/css/main.css... static/css/main.css:187: @media (min-width: 1170px) I assume the plan is to move to one CSS file instead of breaking them down by device size? https://codereview.adblockplus.org/29551738/diff/29557801/static/js/main.js File static/js/main.js (right): https://codereview.adblockplus.org/29551738/diff/29557801/static/js/main.js#n... static/js/main.js:83: if ("querySelector" in document) This if statement is no longer needed since you're using getElementById https://codereview.adblockplus.org/29551738/diff/29557801/static/js/main.js#n... static/js/main.js:97: document.getElementById("navbar-menu").onclick = navigationClick; This if statement is no longer needed since you're using getElementById https://codereview.adblockplus.org/29551738/diff/29557801/templates/default.tmpl File templates/default.tmpl (right): https://codereview.adblockplus.org/29551738/diff/29557801/templates/default.t... templates/default.tmpl:80: <li{{ " class=\"selected\"" if pagename == page else "" }}> `class=\"selected\"` This is resulting in there being double quotes, i.e. class=""selected"". You don't need the inner quotes. https://codereview.adblockplus.org/29551738/diff/29557801/templates/default.t... templates/default.tmpl:80: <li{{ " class=\"selected\"" if pagename == page else "" }}> I don't think you need the else part of this statement https://codereview.adblockplus.org/29551738/diff/29557801/templates/default.t... templates/default.tmpl:80: <li{{ " class=\"selected\"" if pagename == page else "" }}> Theres no styling for selected items https://codereview.adblockplus.org/29551738/diff/29557801/templates/default.t... templates/default.tmpl:89: alt="{{ "ABP" | translate("navbar-logo-alt", "Navbar logo alt text") }}" NIT: Use a more descriptive alt text? https://codereview.adblockplus.org/29551738/diff/29557801/templates/default.t... templates/default.tmpl:91: srcset="/img/navbar-logo-2x.svg 2x"> NIT: Do we typically add -2x modifiers on the SVG versions? https://codereview.adblockplus.org/29551738/diff/29557801/templates/default.t... templates/default.tmpl:99: alt="{{ "Menu icon" | translate("navbar-menu-logo", "Navbar menu logo alt text") }}"> "Menu icon" alone doesn't convey that this will toggle the menubar open/closed. Will this be handled in #5753? https://codereview.adblockplus.org/29551738/diff/29557801/templates/default.t... templates/default.tmpl:99: alt="{{ "Menu icon" | translate("navbar-menu-logo", "Navbar menu logo alt text") }}"> NIT/Suggest: "Navbar menu icon alt text" https://codereview.adblockplus.org/29551738/diff/29557801/templates/default.t... templates/default.tmpl:112: {{ page | linkify(available_locale, class="selected" if available_locale == locale else "") }} Same here. I don't think the `else ""` is necessary
Responses. https://codereview.adblockplus.org/29551738/diff/29557801/static/css/main.css File static/css/main.css (right): https://codereview.adblockplus.org/29551738/diff/29557801/static/css/main.css... static/css/main.css:187: @media (min-width: 1170px) On 2017/09/28 08:38:44, ire wrote: > I assume the plan is to move to one CSS file instead of breaking them down by > device size? Yes. Unless there is a good reason to separate them that I don't know about yet. https://codereview.adblockplus.org/29551738/diff/29557801/static/js/main.js File static/js/main.js (right): https://codereview.adblockplus.org/29551738/diff/29557801/static/js/main.js#n... static/js/main.js:83: if ("querySelector" in document) On 2017/09/28 08:38:44, ire wrote: > This if statement is no longer needed since you're using getElementById Acknowledged. https://codereview.adblockplus.org/29551738/diff/29557801/static/js/main.js#n... static/js/main.js:97: document.getElementById("navbar-menu").onclick = navigationClick; On 2017/09/28 08:38:44, ire wrote: > This if statement is no longer needed since you're using getElementById Acknowledged. https://codereview.adblockplus.org/29551738/diff/29557801/templates/default.tmpl File templates/default.tmpl (right): https://codereview.adblockplus.org/29551738/diff/29557801/templates/default.t... templates/default.tmpl:80: <li{{ " class=\"selected\"" if pagename == page else "" }}> On 2017/09/28 08:38:44, ire wrote: > Theres no styling for selected items Acknowledged. (This is used in the footer too. I was too lazy to check if there were footer specific styles.) https://codereview.adblockplus.org/29551738/diff/29557801/templates/default.t... templates/default.tmpl:80: <li{{ " class=\"selected\"" if pagename == page else "" }}> On 2017/09/28 08:38:45, ire wrote: > `class=\"selected\"` This is resulting in there being double quotes, i.e. > class=""selected"". You don't need the inner quotes. Acknowledged. https://codereview.adblockplus.org/29551738/diff/29557801/templates/default.t... templates/default.tmpl:80: <li{{ " class=\"selected\"" if pagename == page else "" }}> On 2017/09/28 08:38:45, ire wrote: > I don't think you need the else part of this statement ~Not true https://stackoverflow.com/questions/12199757/python-ternary-operator-without-... https://stackoverflow.com/questions/14214942/python-jinja2-shorthand-conditional I'll give the "and" solution a shot. Let me know if you think it's too confusing. https://codereview.adblockplus.org/29551738/diff/29557801/templates/default.t... templates/default.tmpl:89: alt="{{ "ABP" | translate("navbar-logo-alt", "Navbar logo alt text") }}" On 2017/09/28 08:38:46, ire wrote: > NIT: Use a more descriptive alt text? I used "ABP" because it doesn't require translation. I think that we can address this separately. https://codereview.adblockplus.org/29551738/diff/29557801/templates/default.t... templates/default.tmpl:91: srcset="/img/navbar-logo-2x.svg 2x"> On 2017/09/28 08:38:44, ire wrote: > NIT: Do we typically add -2x modifiers on the SVG versions? I don't know (we don't typically support 2x - I did for the first time when I rushed through acceptableads.com). Do you love it or hate it? https://codereview.adblockplus.org/29551738/diff/29557801/templates/default.t... templates/default.tmpl:99: alt="{{ "Menu icon" | translate("navbar-menu-logo", "Navbar menu logo alt text") }}"> On 2017/09/28 08:38:45, ire wrote: > NIT/Suggest: "Navbar menu icon alt text" Acknowledged. https://codereview.adblockplus.org/29551738/diff/29557801/templates/default.t... templates/default.tmpl:99: alt="{{ "Menu icon" | translate("navbar-menu-logo", "Navbar menu logo alt text") }}"> On 2017/09/28 08:38:45, ire wrote: > "Menu icon" alone doesn't convey that this will toggle the menubar open/closed. > Will this be handled in #5753? I will remove this text and address it in #5753. The same goes elsewhere. https://codereview.adblockplus.org/29551738/diff/29557801/templates/default.t... templates/default.tmpl:112: {{ page | linkify(available_locale, class="selected" if available_locale == locale else "") }} On 2017/09/28 08:38:46, ire wrote: > Same here. I don't think the `else ""` is necessary See comment above.
Thanks Julian! https://codereview.adblockplus.org/29551738/diff/29557801/static/css/main.css File static/css/main.css (right): https://codereview.adblockplus.org/29551738/diff/29557801/static/css/main.css... static/css/main.css:187: @media (min-width: 1170px) On 2017/10/04 15:33:28, juliandoucette wrote: > On 2017/09/28 08:38:44, ire wrote: > > I assume the plan is to move to one CSS file instead of breaking them down by > > device size? > > Yes. Unless there is a good reason to separate them that I don't know about yet. I think there are performance reasons (because only the CSS file matching the media query will be loaded initially. The others won't block page render). But I'm not sure if the improvement will be significant enough. Anyway, I'm for us moving to one CSS file, was just asking to make sure we're on the same page. https://codereview.adblockplus.org/29551738/diff/29557801/templates/default.tmpl File templates/default.tmpl (right): https://codereview.adblockplus.org/29551738/diff/29557801/templates/default.t... templates/default.tmpl:80: <li{{ " class=\"selected\"" if pagename == page else "" }}> On 2017/10/04 15:33:29, juliandoucette wrote: > On 2017/09/28 08:38:45, ire wrote: > > I don't think you need the else part of this statement > > ~Not true > > https://stackoverflow.com/questions/12199757/python-ternary-operator-without-... > https://stackoverflow.com/questions/14214942/python-jinja2-shorthand-conditional > > I'll give the "and" solution a shot. Let me know if you think it's too > confusing. It's in the Jinja2 docs: > The else part is optional. If not provided, the else block implicitly evaluates into an undefined object (http://jinja.pocoo.org/docs/2.9/templates/#if-expression) I also tried it myself and it works without just `{{ " class=selected " if pagename == page }}`. I think those answers are to do with just Python, not using Jinja. https://codereview.adblockplus.org/29551738/diff/29557801/templates/default.t... templates/default.tmpl:89: alt="{{ "ABP" | translate("navbar-logo-alt", "Navbar logo alt text") }}" On 2017/10/04 15:33:28, juliandoucette wrote: > On 2017/09/28 08:38:46, ire wrote: > > NIT: Use a more descriptive alt text? > > I used "ABP" because it doesn't require translation. I think that we can address > this separately. Sure. https://codereview.adblockplus.org/29551738/diff/29557801/templates/default.t... templates/default.tmpl:91: srcset="/img/navbar-logo-2x.svg 2x"> On 2017/10/04 15:33:28, juliandoucette wrote: > On 2017/09/28 08:38:44, ire wrote: > > NIT: Do we typically add -2x modifiers on the SVG versions? > > I don't know (we don't typically support 2x - I did for the first time when I > rushed through http://acceptableads.com). > > Do you love it or hate it? Here's my thoughts: 1. I like our other method of separating the files into different folders (png, png2x, svg) which means we don't have to add the 2x modifier on the filenames. 2. Assuming we can't (or don't want to for whatever reason) do that here, I think it makes sense to have the modifiers on the filenames so it's clear. 3. I think -2x on an svg file is misleading because an svg can be any size https://codereview.adblockplus.org/29551738/diff/29557801/templates/default.t... templates/default.tmpl:99: alt="{{ "Menu icon" | translate("navbar-menu-logo", "Navbar menu logo alt text") }}"> On 2017/10/04 15:33:29, juliandoucette wrote: > On 2017/09/28 08:38:45, ire wrote: > > "Menu icon" alone doesn't convey that this will toggle the menubar > open/closed. > > Will this be handled in #5753? > > I will remove this text and address it in #5753. The same goes elsewhere. Acknowledged.
Rebased. Refactoring implementation now.
Ready for review. I think that this is about right. I expect that the font-sizes may change when I refactor them for the whole website to match the styleguide. I also expect to refactor what is common between our navbars into a website-defaults navbar. (I know that I broke the old footer by removing .nav classes in this Patchset. I'm not worried because I do not use these classes in my new footer implementation.)
Added a few notes and removed a merge artifact. https://codereview.adblockplus.org/29551738/diff/29567804/static/css/main-des... File static/css/main-desktop.css (left): https://codereview.adblockplus.org/29551738/diff/29567804/static/css/main-des... static/css/main-desktop.css:168: I didn't need to do this. https://codereview.adblockplus.org/29551738/diff/29567804/templates/default.tmpl File templates/default.tmpl (right): https://codereview.adblockplus.org/29551738/diff/29567804/templates/default.t... templates/default.tmpl:79: {% macro pageitem(pagename) %} I'll get rid of this entirely after the new footer lands. https://codereview.adblockplus.org/29551738/diff/29567804/templates/default.t... templates/default.tmpl:85: <nav id="navbar"> There is an inconsistency between this and my footer implementation. "navbar" vs "site-navbar". I chose *just* "navbar" because "site" *just* adds specificity which is redundant in an ID (which should only be used once). This is not a strong opinion. I kinda like site-* out of habit (I picked this up from WordPress theming). https://codereview.adblockplus.org/29551738/diff/29567804/templates/default.t... templates/default.tmpl:97: src="/img/menu-toggle.png" I'll address file names and folders separately. I chose these to be ~consistent.
Here is a more detailed refactoring. I may have over-commented. https://codereview.adblockplus.org/29551738/diff/29572615/static/css/main-des... File static/css/main-desktop.css (left): https://codereview.adblockplus.org/29551738/diff/29572615/static/css/main-des... static/css/main-desktop.css:168: Removed from PatchSet https://codereview.adblockplus.org/29551738/diff/29572615/static/css/main-des... File static/css/main-desktop.css (right): https://codereview.adblockplus.org/29551738/diff/29572615/static/css/main-des... static/css/main-desktop.css:7: #content, Removed #content from these selectors (because .content accommodates) https://codereview.adblockplus.org/29551738/diff/29572615/static/css/main.css File static/css/main.css (right): https://codereview.adblockplus.org/29551738/diff/29572615/static/css/main.css... static/css/main.css:3: font-family: sans-serif; Removed from PatchSet https://codereview.adblockplus.org/29551738/diff/29572615/static/css/main.css... static/css/main.css:50: * #navbar Moved this below .content https://codereview.adblockplus.org/29551738/diff/29572615/static/css/main.css... static/css/main.css:54: { It's necessary to set height here because overflow:auto would hide the dropdown. https://codereview.adblockplus.org/29551738/diff/29572615/static/css/main.css... static/css/main.css:59: #navbar ul This isn't necessary anymore because of website-defaults reset.css https://codereview.adblockplus.org/29551738/diff/29572615/static/css/main.css... static/css/main.css:65: #navbar li This isn't necessary anymore because of website-defaults reset.css https://codereview.adblockplus.org/29551738/diff/29572615/static/css/main.css... static/css/main.css:70: #navbar .container I'm torn between changing #navbar .container or creating a .navbar-container like you did. https://codereview.adblockplus.org/29551738/diff/29572615/static/css/main.css... static/css/main.css:98: text-decoration: none; Removed unnecessary text-decoration nullification. https://codereview.adblockplus.org/29551738/diff/29572615/static/css/main.css... static/css/main.css:127: #logo > span IDK if this looks different on different platforms. https://codereview.adblockplus.org/29551738/diff/29572615/static/css/main.css... static/css/main.css:247: #locale-menu.visible Moved this logic above as it now applies to both desktop and mobile. https://codereview.adblockplus.org/29551738/diff/29572615/static/css/main.css... static/css/main.css:263: line-height: 3em; Changed to default line-height and padding: 0.75em 1em instead of nowrapping. Now items could wrap comfortably (but they won't because there is enough width). https://codereview.adblockplus.org/29551738/diff/29572615/static/css/main.css... static/css/main.css:264: text-decoration: none; Removed unnecessary text-decoration nullification. https://codereview.adblockplus.org/29551738/diff/29572615/static/css/main.css... static/css/main.css:275: #locale-selected::after I think I'll flip this caret on select separately. https://codereview.adblockplus.org/29551738/diff/29572615/templates/default.tmpl File templates/default.tmpl (right): https://codereview.adblockplus.org/29551738/diff/29572615/templates/default.t... templates/default.tmpl:80: {% macro pageitem(pagename) %} Removed from PatchSet (I will remove this after landing header and footer). https://codereview.adblockplus.org/29551738/diff/29572615/templates/default.t... templates/default.tmpl:88: {{ "index" | linkify(id="logo") }} Changed to #navbar-logo https://codereview.adblockplus.org/29551738/diff/29572615/templates/default.t... templates/default.tmpl:90: alt="{{ "ABP" | translate("navbar-logo-alt", "Navbar logo alt text") }}" Removed alt, will re-add separately https://codereview.adblockplus.org/29551738/diff/29572615/templates/default.t... templates/default.tmpl:95: <a href="#" id="menu-toggle"> Changed to #navbar-menu-toggle https://codereview.adblockplus.org/29551738/diff/29572615/templates/default.t... templates/default.tmpl:97: height="25px" Removed height set via css https://codereview.adblockplus.org/29551738/diff/29572615/templates/default.t... templates/default.tmpl:100: alt="{{ "Menu icon" | translate("navbar-menu-logo", "Navbar menu logo alt text") }}"> Removed alt, will re-add separately. https://codereview.adblockplus.org/29551738/diff/29572615/templates/default.t... templates/default.tmpl:104: {{ pageitem(pagename) }} Replaced pageitem usage with code above. https://codereview.adblockplus.org/29551738/diff/29572615/templates/default.t... templates/default.tmpl:108: {{ config.get("langnames", locale) }} ({{ locale | to_og_locale | to_og_location }}) Added "navbar-" before "locale" in these ids
I forgot to mention that I changed the mobile language selector into a sub-dropdown instead of an inline list like it is today. Keep in mind that I am going to completely re-implement the mobile dropdown separately (because the new design specifies a ~pull-right menu instead of a pull-down menu).
On 2017/10/10 17:42:17, juliandoucette wrote: > I forgot to mention that I changed the mobile language selector into a > sub-dropdown instead of an inline list like it is today. Keep in mind that I am > going to completely re-implement the mobile dropdown separately (because the new > design specifies a ~pull-right menu instead of a pull-down menu). Ack.
On 2017/10/06 16:17:16, juliandoucette wrote: > Ready for review. > > I think that this is about right. I expect that the font-sizes may change when I > refactor them for the whole website to match the styleguide. I also expect to > refactor what is common between our navbars into a website-defaults navbar. > > (I know that I broke the old footer by removing .nav classes in this Patchset. > I'm not worried because I do not use these classes in my new footer > implementation.) Ack.
Thanks Julian. A few relatively minor things. You can fix them now or in separate issues later. LGTM https://codereview.adblockplus.org/29551738/diff/29567804/static/css/main-des... File static/css/main-desktop.css (left): https://codereview.adblockplus.org/29551738/diff/29567804/static/css/main-des... static/css/main-desktop.css:168: On 2017/10/06 16:28:56, juliandoucette wrote: > I didn't need to do this. Acknowledged. https://codereview.adblockplus.org/29551738/diff/29567804/templates/default.tmpl File templates/default.tmpl (right): https://codereview.adblockplus.org/29551738/diff/29567804/templates/default.t... templates/default.tmpl:85: <nav id="navbar"> On 2017/10/06 16:28:57, juliandoucette wrote: > There is an inconsistency between this and my footer implementation. "navbar" vs > "site-navbar". I chose *just* "navbar" because "site" *just* adds specificity > which is redundant in an ID (which should only be used once). This is not a > strong opinion. I kinda like site-* out of habit (I picked this up from > WordPress theming). I personally prefer the site- prefix because it tells me this is a global site-wide component, regardless of it only being used once. Since we plan on adding using a .navbar class, I think it would be clearer if this name was changed to something else. But you don't have to do this now (or at all). https://codereview.adblockplus.org/29551738/diff/29567804/templates/default.t... templates/default.tmpl:97: src="/img/menu-toggle.png" On 2017/10/06 16:28:57, juliandoucette wrote: > I'll address file names and folders separately. I chose these to be ~consistent. Acknowledged. https://codereview.adblockplus.org/29551738/diff/29572615/static/css/main-des... File static/css/main-desktop.css (left): https://codereview.adblockplus.org/29551738/diff/29572615/static/css/main-des... static/css/main-desktop.css:168: On 2017/10/10 17:35:04, juliandoucette wrote: > Removed from PatchSet Acknowledged. https://codereview.adblockplus.org/29551738/diff/29572615/static/css/main-des... File static/css/main-desktop.css (right): https://codereview.adblockplus.org/29551738/diff/29572615/static/css/main-des... static/css/main-desktop.css:7: #content, On 2017/10/10 17:35:04, juliandoucette wrote: > Removed #content from these selectors (because .content accommodates) Acknowledged. https://codereview.adblockplus.org/29551738/diff/29572615/static/css/main.css File static/css/main.css (right): https://codereview.adblockplus.org/29551738/diff/29572615/static/css/main.css... static/css/main.css:3: font-family: sans-serif; On 2017/10/10 17:35:04, juliandoucette wrote: > Removed from PatchSet Acknowledged. https://codereview.adblockplus.org/29551738/diff/29572615/static/css/main.css... static/css/main.css:50: * #navbar On 2017/10/10 17:35:08, juliandoucette wrote: > Moved this below .content Acknowledged. https://codereview.adblockplus.org/29551738/diff/29572615/static/css/main.css... static/css/main.css:54: { On 2017/10/10 17:35:06, juliandoucette wrote: > It's necessary to set height here because overflow:auto would hide the dropdown. Acknowledged. https://codereview.adblockplus.org/29551738/diff/29572615/static/css/main.css... static/css/main.css:59: #navbar ul On 2017/10/10 17:35:08, juliandoucette wrote: > This isn't necessary anymore because of website-defaults reset.css Acknowledged. https://codereview.adblockplus.org/29551738/diff/29572615/static/css/main.css... static/css/main.css:65: #navbar li On 2017/10/10 17:35:05, juliandoucette wrote: > This isn't necessary anymore because of website-defaults reset.css Acknowledged. https://codereview.adblockplus.org/29551738/diff/29572615/static/css/main.css... static/css/main.css:70: #navbar .container On 2017/10/10 17:35:08, juliandoucette wrote: > I'm torn between changing #navbar .container or creating a .navbar-container > like you did. I think I prefer creating a new class because it is obvious from looking at the markup that this is different to the regular .container https://codereview.adblockplus.org/29551738/diff/29572615/static/css/main.css... static/css/main.css:98: text-decoration: none; On 2017/10/10 17:35:07, juliandoucette wrote: > Removed unnecessary text-decoration nullification. Acknowledged. https://codereview.adblockplus.org/29551738/diff/29572615/static/css/main.css... static/css/main.css:247: #locale-menu.visible On 2017/10/10 17:35:06, juliandoucette wrote: > Moved this logic above as it now applies to both desktop and mobile. Acknowledged. https://codereview.adblockplus.org/29551738/diff/29572615/static/css/main.css... static/css/main.css:263: line-height: 3em; On 2017/10/10 17:35:05, juliandoucette wrote: > Changed to default line-height and padding: 0.75em 1em instead of nowrapping. > Now items could wrap comfortably (but they won't because there is enough width). Acknowledged. https://codereview.adblockplus.org/29551738/diff/29572615/static/css/main.css... static/css/main.css:264: text-decoration: none; On 2017/10/10 17:35:09, juliandoucette wrote: > Removed unnecessary text-decoration nullification. Acknowledged. https://codereview.adblockplus.org/29551738/diff/29572615/static/css/main.css... static/css/main.css:275: #locale-selected::after On 2017/10/10 17:35:05, juliandoucette wrote: > I think I'll flip this caret on select separately. Acknowledged. https://codereview.adblockplus.org/29551738/diff/29572615/templates/default.tmpl File templates/default.tmpl (right): https://codereview.adblockplus.org/29551738/diff/29572615/templates/default.t... templates/default.tmpl:80: {% macro pageitem(pagename) %} On 2017/10/10 17:35:10, juliandoucette wrote: > Removed from PatchSet (I will remove this after landing header and footer). Acknowledged. https://codereview.adblockplus.org/29551738/diff/29572615/templates/default.t... templates/default.tmpl:88: {{ "index" | linkify(id="logo") }} On 2017/10/10 17:35:11, juliandoucette wrote: > Changed to #navbar-logo Acknowledged. https://codereview.adblockplus.org/29551738/diff/29572615/templates/default.t... templates/default.tmpl:90: alt="{{ "ABP" | translate("navbar-logo-alt", "Navbar logo alt text") }}" On 2017/10/10 17:35:10, juliandoucette wrote: > Removed alt, will re-add separately Acknowledged. https://codereview.adblockplus.org/29551738/diff/29572615/templates/default.t... templates/default.tmpl:95: <a href="#" id="menu-toggle"> On 2017/10/10 17:35:10, juliandoucette wrote: > Changed to #navbar-menu-toggle Acknowledged. https://codereview.adblockplus.org/29551738/diff/29572615/templates/default.t... templates/default.tmpl:97: height="25px" On 2017/10/10 17:35:09, juliandoucette wrote: > Removed height set via css Acknowledged. https://codereview.adblockplus.org/29551738/diff/29572615/templates/default.t... templates/default.tmpl:100: alt="{{ "Menu icon" | translate("navbar-menu-logo", "Navbar menu logo alt text") }}"> On 2017/10/10 17:35:09, juliandoucette wrote: > Removed alt, will re-add separately. Acknowledged. https://codereview.adblockplus.org/29551738/diff/29572615/templates/default.t... templates/default.tmpl:104: {{ pageitem(pagename) }} On 2017/10/10 17:35:10, juliandoucette wrote: > Replaced pageitem usage with code above. Acknowledged. https://codereview.adblockplus.org/29551738/diff/29572615/templates/default.t... templates/default.tmpl:108: {{ config.get("langnames", locale) }} ({{ locale | to_og_locale | to_og_location }}) On 2017/10/10 17:35:09, juliandoucette wrote: > Added "navbar-" before "locale" in these ids Acknowledged. https://codereview.adblockplus.org/29551738/diff/29572803/static/css/main.css File static/css/main.css (right): https://codereview.adblockplus.org/29551738/diff/29572803/static/css/main.css... static/css/main.css:284: display: block; The display doesn't do anything since you are floating https://codereview.adblockplus.org/29551738/diff/29572803/static/css/main.css... static/css/main.css:299: margin-right: 1em; This margin-right still applies when direction is rtl https://codereview.adblockplus.org/29551738/diff/29572803/static/css/main.css... static/css/main.css:309: /* full-height: 2.91 * 1.375 = ~4em */ Helpful comments, thanks :) https://codereview.adblockplus.org/29551738/diff/29572803/static/css/main.css... static/css/main.css:371: #navbar-locale-selected::after Single colon for :after https://codereview.adblockplus.org/29551738/diff/29572803/static/css/main.css... static/css/main.css:376: margin-left: .255em; "Don't omit the optional leading 0 for decimal numbers." https://codereview.adblockplus.org/29551738/diff/29572803/static/css/main.css... static/css/main.css:458: z-index: 1000; NIT: A lower z-index (e.g. even 1 or 2) would suffice here
Addressed comments
https://codereview.adblockplus.org/29551738/diff/29572615/static/css/main.css File static/css/main.css (right): https://codereview.adblockplus.org/29551738/diff/29572615/static/css/main.css... static/css/main.css:70: #navbar .container On 2017/10/11 08:09:00, ire wrote: > On 2017/10/10 17:35:08, juliandoucette wrote: > > I'm torn between changing #navbar .container or creating a .navbar-container > > like you did. > > I think I prefer creating a new class because it is obvious from looking at the > markup that this is different to the regular .container Agreed. I'll change it. https://codereview.adblockplus.org/29551738/diff/29572803/static/css/main.css File static/css/main.css (right): https://codereview.adblockplus.org/29551738/diff/29572803/static/css/main.css... static/css/main.css:284: display: block; On 2017/10/11 08:09:08, ire wrote: > The display doesn't do anything since you are floating I didn't know that :D - Thank you. https://codereview.adblockplus.org/29551738/diff/29572803/static/css/main.css... static/css/main.css:299: margin-right: 1em; On 2017/10/11 08:09:09, ire wrote: > This margin-right still applies when direction is rtl Good catch! https://codereview.adblockplus.org/29551738/diff/29572803/static/css/main.css... static/css/main.css:309: /* full-height: 2.91 * 1.375 = ~4em */ On 2017/10/11 08:09:07, ire wrote: > Helpful comments, thanks :) You're welcome. https://codereview.adblockplus.org/29551738/diff/29572803/static/css/main.css... static/css/main.css:371: #navbar-locale-selected::after On 2017/10/11 08:09:07, ire wrote: > Single colon for :after Done. https://codereview.adblockplus.org/29551738/diff/29572803/static/css/main.css... static/css/main.css:376: margin-left: .255em; On 2017/10/11 08:09:09, ire wrote: > "Don't omit the optional leading 0 for decimal numbers." Good catch. Looks like I neglected rtl too. https://codereview.adblockplus.org/29551738/diff/29572803/static/css/main.css... static/css/main.css:458: z-index: 1000; On 2017/10/11 08:09:09, ire wrote: > NIT: A lower z-index (e.g. even 1 or 2) would suffice here pfft
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
