Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 29485575: Issue 5385 - Create Site Header Component for Help Center (Closed)

Created:
July 10, 2017, 4:08 p.m. by ire
Modified:
Sept. 4, 2017, 1:34 p.m.
Reviewers:
juliandoucette
Visibility:
Public.

Description

Issue 5385 - Create Site Header Component for Help Center

Patch Set 1 #

Total comments: 39

Patch Set 2 : Fix svgs, Implement search, Show searchbar for no-js #

Total comments: 92

Patch Set 3 : Addressed smaller comments #

Patch Set 4 : Rebase #

Patch Set 5 : Unfix header, implement standard spacing units #

Total comments: 51

Patch Set 6 : Addressed NITs #

Patch Set 7 : Fix alignment of search form, reformat JS #

Total comments: 57

Patch Set 8 : Implement navbar-collapse #

Total comments: 16

Patch Set 9 : Change mobile to tablet-breakpoint, add polyfill for classList, re-order rtl styles #

Patch Set 10 : Include site variables before website-defaults #

Unified diffs Side-by-side diffs Delta from patch set Stats (+513 lines, -31 lines) Patch
A includes/layout/header.tmpl View 1 2 3 4 5 6 7 8 1 chunk +30 lines, -0 lines 0 comments Download
A static/img/png/external-icon.png View 1 2 3 Binary file 0 comments Download
A static/img/png/eyeo-help.png View Binary file 0 comments Download
A static/img/png/search-icon.png View 1 2 Binary file 0 comments Download
A static/img/svg/external-icon.svg View 1 2 3 1 chunk +13 lines, -0 lines 0 comments Download
A static/img/svg/eyeo-help.svg View 1 1 chunk +19 lines, -0 lines 0 comments Download
A static/img/svg/search-icon.svg View 1 2 1 chunk +8 lines, -0 lines 0 comments Download
A static/js/main.js View 1 2 3 4 5 6 7 1 chunk +27 lines, -0 lines 0 comments Download
A static/js/vendor/classList.min.js View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -0 lines 0 comments Download
M static/scss/base/_font.scss View 1 2 3 1 chunk +18 lines, -21 lines 0 comments Download
M static/scss/base/_reset.scss View 1 2 3 1 chunk +7 lines, -6 lines 0 comments Download
A static/scss/base/_utilities.scss View 1 2 3 4 5 6 7 8 9 1 chunk +88 lines, -0 lines 0 comments Download
M static/scss/base/_variables.scss View 1 2 3 4 5 6 7 8 9 2 chunks +31 lines, -1 line 0 comments Download
A static/scss/components/_search-form.scss View 1 2 3 4 5 6 7 8 1 chunk +128 lines, -0 lines 0 comments Download
A static/scss/layout/_header.scss View 1 2 3 4 5 6 7 8 1 chunk +66 lines, -0 lines 0 comments Download
A static/scss/layout/_navbar.scss View 1 2 3 4 5 6 7 1 chunk +63 lines, -0 lines 0 comments Download
M static/scss/main.scss View 1 2 3 4 5 6 7 8 9 1 chunk +5 lines, -1 line 0 comments Download
M templates/default.tmpl View 1 2 3 4 5 6 7 8 1 chunk +8 lines, -2 lines 0 comments Download

Messages

Total messages: 25
ire
July 10, 2017, 4:08 p.m. (2017-07-10 16:08:51 UTC) #1
ire
First attempt. I was mostly concerned with the layout of things on mobile vs desktop. ...
July 10, 2017, 4:18 p.m. (2017-07-10 16:18:02 UTC) #2
juliandoucette
First impressions - I hope they help :) https://codereview.adblockplus.org/29485575/diff/29485576/.hgignore File .hgignore (right): https://codereview.adblockplus.org/29485575/diff/29485576/.hgignore#newcode3 .hgignore:3: .stylelintrc.json ...
July 14, 2017, 2:11 p.m. (2017-07-14 14:11:30 UTC) #3
ire
> First impressions - I hope they help :) Thanks they did :) I think ...
July 17, 2017, 9:25 a.m. (2017-07-17 09:25:54 UTC) #4
ire
https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/base/_utilities.scss File static/scss/base/_utilities.scss (right): https://codereview.adblockplus.org/29485575/diff/29490566/static/scss/base/_utilities.scss#newcode82 static/scss/base/_utilities.scss:82: .unstyled It just occurred to me that I'm replicating ...
July 17, 2017, 9:31 a.m. (2017-07-17 09:31:38 UTC) #5
juliandoucette
Full review this time. Let me know if you want to meet to go over ...
July 19, 2017, 6:03 p.m. (2017-07-19 18:03:39 UTC) #6
ire
Thanks Julian! I've done my best to address most of the smaller comments that I ...
July 21, 2017, 10:23 a.m. (2017-07-21 10:23:32 UTC) #7
juliandoucette
Quick note: Let's make sure our search form supports https://developers.google.com/search/docs/data-types/sitelinks-searchbox
July 22, 2017, 4:54 p.m. (2017-07-22 16:54:12 UTC) #8
juliandoucette
Thanks Ire! Partial reviews/responses below. https://codereview.adblockplus.org/29485575/diff/29490566/includes/searchform.tmpl File includes/searchform.tmpl (right): https://codereview.adblockplus.org/29485575/diff/29490566/includes/searchform.tmpl#newcode6 includes/searchform.tmpl:6: <img src="/img/png/searchIcon.png" srcset="/img/png/searchIcon.png, /img/svg/searchIcon.svg ...
July 24, 2017, 9:08 p.m. (2017-07-24 21:08:11 UTC) #9
juliandoucette
Can you rebase this onto the tip of help.eyeo.com?
Aug. 9, 2017, 3:52 p.m. (2017-08-09 15:52:14 UTC) #10
ire
> Can you rebase this onto the tip of help.eyeo.com? Done + plus some smaller ...
Aug. 10, 2017, 4:54 p.m. (2017-08-10 16:54:44 UTC) #11
ire
I spoke with Jeen to clarify some of the things you asked about. Notes below. ...
Aug. 16, 2017, 2:41 p.m. (2017-08-16 14:41:27 UTC) #12
juliandoucette
Thanks Ire, All minor issues IMO. Don't worry about updating this soon if there are ...
Aug. 18, 2017, 3:03 p.m. (2017-08-18 15:03:58 UTC) #13
ire
> Thanks Ire, > > All minor issues IMO. Don't worry about updating this soon ...
Aug. 21, 2017, 3:23 p.m. (2017-08-21 15:23:24 UTC) #14
juliandoucette
Here are my responses to your comments. https://codereview.adblockplus.org/29485575/diff/29517664/includes/layout/header.tmpl File includes/layout/header.tmpl (right): https://codereview.adblockplus.org/29485575/diff/29517664/includes/layout/header.tmpl#newcode8 includes/layout/header.tmpl:8: <button id="toggle-search-form" ...
Aug. 21, 2017, 4:28 p.m. (2017-08-21 16:28:42 UTC) #15
ire
https://codereview.adblockplus.org/29485575/diff/29517664/includes/layout/header.tmpl File includes/layout/header.tmpl (right): https://codereview.adblockplus.org/29485575/diff/29517664/includes/layout/header.tmpl#newcode16 includes/layout/header.tmpl:16: <img src="/img/png/external-icon.png" srcset="/img/svg/external-icon.svg 2x" alt="{{ "External link icon" | ...
Aug. 22, 2017, 12:15 p.m. (2017-08-22 12:15:17 UTC) #16
juliandoucette
Is this ready for review again?
Aug. 22, 2017, 3:02 p.m. (2017-08-22 15:02:53 UTC) #17
ire
On 2017/08/22 15:02:53, juliandoucette wrote: > Is this ready for review again? Yes
Aug. 22, 2017, 4:40 p.m. (2017-08-22 16:40:03 UTC) #18
juliandoucette
I didn't go into _searchform.scs or _header.scss because these may change significantly if you agree ...
Aug. 23, 2017, 2:39 p.m. (2017-08-23 14:39:11 UTC) #19
ire
Thanks Julian :) Made some changes https://codereview.adblockplus.org/29485575/diff/29523654/includes/layout/header.tmpl File includes/layout/header.tmpl (right): https://codereview.adblockplus.org/29485575/diff/29523654/includes/layout/header.tmpl#newcode2 includes/layout/header.tmpl:2: <div class="navbar-wrapper"> On ...
Aug. 25, 2017, 8:27 a.m. (2017-08-25 08:27:15 UTC) #20
juliandoucette
https://codereview.adblockplus.org/29485575/diff/29523654/includes/search-form.tmpl File includes/search-form.tmpl (right): https://codereview.adblockplus.org/29485575/diff/29523654/includes/search-form.tmpl#newcode2 includes/search-form.tmpl:2: <label for="search" class="sr-only">{{ "Search Adblock Plus Help" | translate("search-form-label", ...
Aug. 29, 2017, 4:17 p.m. (2017-08-29 16:17:33 UTC) #21
ire
Updates ready. https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/base/_utilities.scss File static/scss/base/_utilities.scss (right): https://codereview.adblockplus.org/29485575/diff/29523654/static/scss/base/_utilities.scss#newcode50 static/scss/base/_utilities.scss:50: @media (min-width: $mobile-breakpoint) On 2017/08/29 16:17:24, juliandoucette ...
Sept. 1, 2017, 10:57 a.m. (2017-09-01 10:57:25 UTC) #22
juliandoucette
I think we should push this after you address the issues below. I found a ...
Sept. 1, 2017, 11:43 a.m. (2017-09-01 11:43:46 UTC) #23
ire
> I think we should push this after you address the issues below. I found ...
Sept. 1, 2017, 12:12 p.m. (2017-09-01 12:12:06 UTC) #24
juliandoucette
Sept. 1, 2017, 12:32 p.m. (2017-09-01 12:32:28 UTC) #25
LGTM

Powered by Google App Engine
This is Rietveld