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

Issue 29548567: Issue 4925 - Create accordion component for Help Center (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 1 month ago by ire
Modified:
2 years, 1 month ago
Reviewers:
juliandoucette
Base URL:
https://hg.adblockplus.org/help.eyeo.com
Visibility:
Public.

Description

Issue 4925 - Create accordion component for Help Center

Patch Set 1 #

Total comments: 5

Patch Set 2 : Add support for navigating between headings with arrow keys #

Patch Set 3 : Rebase #

Total comments: 40

Patch Set 4 : Rebase, Accordion object, hover state #

Total comments: 6

Patch Set 5 : Refactoring #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+250 lines, -0 lines) Patch
M pages/index.md View 1 2 3 1 chunk +40 lines, -0 lines 0 comments Download
A static/img/png/arrow-icon-secondary.png View Binary file 0 comments Download
A static/img/svg/arrow-icon-secondary.svg View 1 chunk +10 lines, -0 lines 0 comments Download
M static/js/main.js View 1 2 3 4 1 chunk +116 lines, -0 lines 1 comment Download
A static/scss/components/_accordion.scss View 1 2 3 1 chunk +83 lines, -0 lines 0 comments Download
M static/scss/main.scss View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 16
ire
2 years, 1 month ago (2017-09-18 10:03:13 UTC) #1
ire
Ready for review. In this implementation I did not add any relevant content. Just placeholders. ...
2 years, 1 month ago (2017-09-18 10:08:11 UTC) #2
juliandoucette
Looks good at first glance. I have a few questions before I dig deeper though: ...
2 years, 1 month ago (2017-09-18 12:53:31 UTC) #3
ire
On 2017/09/18 12:53:31, juliandoucette wrote: > Looks good at first glance. I have a few ...
2 years, 1 month ago (2017-09-18 14:57:02 UTC) #4
juliandoucette
On 2017/09/18 14:57:02, ire wrote: > Based on me using the outdated example, would you ...
2 years, 1 month ago (2017-09-18 15:49:04 UTC) #5
ire
On 2017/09/18 15:49:04, juliandoucette wrote: > On 2017/09/18 14:57:02, ire wrote: > > Based on ...
2 years, 1 month ago (2017-09-19 09:15:31 UTC) #6
juliandoucette
This Patchset doesn't seem to apply?
2 years, 1 month ago (2017-09-19 14:11:26 UTC) #7
ire
On 2017/09/19 14:11:26, juliandoucette wrote: > This Patchset doesn't seem to apply? Rebase. Sorry about ...
2 years, 1 month ago (2017-09-19 16:19:54 UTC) #8
juliandoucette
Note: The only non-NIT / non-suggestion is the aria-disabled issue. https://codereview.adblockplus.org/29548567/diff/29549875/pages/index.md File pages/index.md (right): https://codereview.adblockplus.org/29548567/diff/29549875/pages/index.md#newcode22 ...
2 years, 1 month ago (2017-09-20 14:19:39 UTC) #9
juliandoucette
One more thing before I forget: I suggest that we add a more distinct hover/focus/active ...
2 years, 1 month ago (2017-09-20 14:21:35 UTC) #10
ire
Thanks! > One more thing before I forget: I suggest that we add a more ...
2 years, 1 month ago (2017-09-25 11:25:19 UTC) #11
juliandoucette
https://codereview.adblockplus.org/29548567/diff/29549875/static/js/main.js File static/js/main.js (right): https://codereview.adblockplus.org/29548567/diff/29549875/static/js/main.js#newcode55 static/js/main.js:55: function setupAccordionMenu(accordionEl) On 2017/09/25 11:25:18, ire wrote: > On ...
2 years, 1 month ago (2017-09-26 18:57:06 UTC) #12
ire
Updated. https://codereview.adblockplus.org/29548567/diff/29549875/static/js/main.js File static/js/main.js (right): https://codereview.adblockplus.org/29548567/diff/29549875/static/js/main.js#newcode55 static/js/main.js:55: function setupAccordionMenu(accordionEl) On 2017/09/26 18:57:05, juliandoucette wrote: > ...
2 years, 1 month ago (2017-09-28 14:48:53 UTC) #13
juliandoucette
LGTM - But I was unable to test because the patch does not apply. Please ...
2 years, 1 month ago (2017-10-09 21:18:17 UTC) #14
juliandoucette
Scratch that - I was able to get this to work on -r 7. Still ...
2 years, 1 month ago (2017-10-09 21:24:28 UTC) #15
ire
2 years, 1 month ago (2017-10-10 12:13:16 UTC) #16
On 2017/10/09 21:24:28, juliandoucette wrote:
> Scratch that - I was able to get this to work on -r 7. Still LGTM - But I
think
> that the dark grey hover/active/focus state is way too dark :/ ?

I created an issue for this https://issues.adblockplus.org/ticket/5852#ticket
Sign in to reply to this message.

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