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

Issue 318910043: Implement sidebar folders as recursive tree structure. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
7 years, 3 months ago by angelayang
Modified:
7 years, 3 months ago
Reviewers:
tsergeant, calamity
CC:
calamity+md_bookmarks_chromium.org, tsergeant+md_bookmarks_chromium.org, jiaxi
Visibility:
Public.

Description

Implement sidebar folders as recursive tree structure. Add folder-node custom element. Folders close and open on drop down icon tap. Selected folder is highlighted blue. BUG= R=calamity@chromium.org, tsergeant@chromium.org Committed: 9237b888171bc235ca023b841ea7bedfe3895b54

Patch Set 1 #

Total comments: 12

Patch Set 2 : Rebase #

Patch Set 3 : Added closure annotations #

Patch Set 4 : Fix Closure Annotations #

Total comments: 29

Patch Set 5 : Css cleanup. #

Total comments: 30

Patch Set 6 : Fix for Tim's comments. #

Total comments: 10

Patch Set 7 : Resize dropdown. #

Total comments: 2

Patch Set 8 : Remove relative position for folder node in tree. #

Patch Set 9 : Set variable as local not global. #

Total comments: 12

Patch Set 10 : Code clean up. #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+240 lines, -28 lines) Patch
M chrome/browser/browser_resources.grd View 1 2 3 4 5 6 7 8 9 1 chunk +5 lines, -2 lines 0 comments Download
M chrome/browser/resources/md_bookmarks/dummy_data.js View 1 6 1 chunk +16 lines, -3 lines 0 comments Download
A chrome/browser/resources/md_bookmarks/folder_node.html View 1 2 3 4 5 6 7 8 9 1 chunk +86 lines, -0 lines 0 comments Download
A chrome/browser/resources/md_bookmarks/folder_node.js View 1 2 3 4 5 6 7 8 1 chunk +58 lines, -0 lines 0 comments Download
A chrome/browser/resources/md_bookmarks/icons.html View 1 2 1 chunk +14 lines, -0 lines 0 comments Download
M chrome/browser/resources/md_bookmarks/shared_style.html View 1 2 3 4 5 6 7 8 9 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/browser/resources/md_bookmarks/shared_vars.html View 1 2 3 4 5 6 7 8 9 1 chunk +6 lines, -3 lines 1 comment Download
M chrome/browser/resources/md_bookmarks/sidebar.html View 1 2 3 4 5 6 1 chunk +8 lines, -17 lines 0 comments Download
M chrome/browser/resources/md_bookmarks/sidebar.js View 1 2 3 4 5 6 7 8 9 1 chunk +35 lines, -0 lines 0 comments Download
M chrome/browser/resources/settings/icons.html View 1 2 3 4 5 1 chunk +0 lines, -1 line 0 comments Download
M chrome/browser/resources/settings/settings_main/settings_main.js View 1 2 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/resources/settings/settings_menu/settings_menu.js View 1 2 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/webui/md_bookmarks/md_bookmarks_ui.cc View 1 2 3 4 5 6 7 8 9 1 chunk +5 lines, -0 lines 0 comments Download
M ui/webui/resources/cr_elements/icons.html View 1 2 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 16
angelayang
Hello, I've put up a sidebar patch. Review when you get a chance? Thankyou!
7 years, 3 months ago (2016-12-05 06:43:56 UTC) #1
tsergeant
Charlotte committed a patch which adds a shared_style.html and conflicts with yours. Can you please ...
7 years, 3 months ago (2016-12-06 00:20:39 UTC) #2
angelayang
Hello, I've added closure annotations. https://codereview.appspot.com/318910043/diff/1/chrome/browser/resources/md_bookmarks/bookmarks.html File chrome/browser/resources/md_bookmarks/bookmarks.html (left): https://codereview.appspot.com/318910043/diff/1/chrome/browser/resources/md_bookmarks/bookmarks.html#oldcode11 chrome/browser/resources/md_bookmarks/bookmarks.html:11: On 2016/12/06 00:20:40, tsergeant ...
7 years, 3 months ago (2016-12-06 03:35:09 UTC) #3
calamity
https://codereview.appspot.com/318910043/diff/50014/chrome/browser/resources/md_bookmarks/bookmarks.html File chrome/browser/resources/md_bookmarks/bookmarks.html (right): https://codereview.appspot.com/318910043/diff/50014/chrome/browser/resources/md_bookmarks/bookmarks.html#newcode11 chrome/browser/resources/md_bookmarks/bookmarks.html:11: lol https://codereview.appspot.com/318910043/diff/50014/chrome/browser/resources/md_bookmarks/folder_node.html File chrome/browser/resources/md_bookmarks/folder_node.html (right): https://codereview.appspot.com/318910043/diff/50014/chrome/browser/resources/md_bookmarks/folder_node.html#newcode17 chrome/browser/resources/md_bookmarks/folder_node.html:17: .container { ...
7 years, 3 months ago (2016-12-06 04:34:53 UTC) #4
angelayang
Hi I have fixed the code according to Chris's comments. Please take a peak :) ...
7 years, 3 months ago (2016-12-06 23:23:01 UTC) #5
tsergeant
Good job overall 👌 Comments are a mix of style nitpicks, RTL problems, and suggestions ...
7 years, 3 months ago (2016-12-07 02:19:45 UTC) #6
angelayang
Hello, I've made fixes. Please take a peek? thankyou https://codereview.appspot.com/318910043/diff/70001/chrome/browser/browser_resources.grd File chrome/browser/browser_resources.grd (right): https://codereview.appspot.com/318910043/diff/70001/chrome/browser/browser_resources.grd#newcode257 chrome/browser/browser_resources.grd:257: ...
7 years, 3 months ago (2016-12-07 03:55:59 UTC) #7
tsergeant
https://codereview.appspot.com/318910043/diff/90001/chrome/browser/resources/md_bookmarks/dummy_data.js File chrome/browser/resources/md_bookmarks/dummy_data.js (right): https://codereview.appspot.com/318910043/diff/90001/chrome/browser/resources/md_bookmarks/dummy_data.js#newcode23 chrome/browser/resources/md_bookmarks/dummy_data.js:23: 'name' : 'Classicaaskdhjlsaijhfolijdsoigjofdjsgdflkgj', My favourite design style. https://codereview.appspot.com/318910043/diff/90001/chrome/browser/resources/md_bookmarks/folder_node.html File ...
7 years, 3 months ago (2016-12-07 04:59:14 UTC) #8
angelayang
Hello, One more time! Thankyou https://codereview.appspot.com/318910043/diff/90001/chrome/browser/resources/md_bookmarks/dummy_data.js File chrome/browser/resources/md_bookmarks/dummy_data.js (right): https://codereview.appspot.com/318910043/diff/90001/chrome/browser/resources/md_bookmarks/dummy_data.js#newcode23 chrome/browser/resources/md_bookmarks/dummy_data.js:23: 'name' : 'Classicaaskdhjlsaijhfolijdsoigjofdjsgdflkgj', On ...
7 years, 3 months ago (2016-12-07 05:41:51 UTC) #9
tsergeant
lgtm! https://codereview.appspot.com/318910043/diff/110001/chrome/browser/resources/md_bookmarks/folder_node.html File chrome/browser/resources/md_bookmarks/folder_node.html (right): https://codereview.appspot.com/318910043/diff/110001/chrome/browser/resources/md_bookmarks/folder_node.html#newcode14 chrome/browser/resources/md_bookmarks/folder_node.html:14: position: relative; Now that you've gotten rid of ...
7 years, 3 months ago (2016-12-07 23:02:02 UTC) #10
angelayang
all done :) https://codereview.appspot.com/318910043/diff/110001/chrome/browser/resources/md_bookmarks/folder_node.html File chrome/browser/resources/md_bookmarks/folder_node.html (right): https://codereview.appspot.com/318910043/diff/110001/chrome/browser/resources/md_bookmarks/folder_node.html#newcode14 chrome/browser/resources/md_bookmarks/folder_node.html:14: position: relative; On 2016/12/07 23:02:02, tsergeant ...
7 years, 3 months ago (2016-12-08 01:57:40 UTC) #11
angelayang
all done :)
7 years, 3 months ago (2016-12-08 01:57:44 UTC) #12
calamity
https://codereview.appspot.com/318910043/diff/130002/chrome/browser/browser_resources.grd File chrome/browser/browser_resources.grd (right): https://codereview.appspot.com/318910043/diff/130002/chrome/browser/browser_resources.grd#newcode256 chrome/browser/browser_resources.grd:256: <include name="IDR_MD_BOOKMARKS_FOLDER_NODE_JS" file="resources\md_bookmarks\folder_node.js" type="BINDATA" /> Sort. https://codereview.appspot.com/318910043/diff/130002/chrome/browser/resources/md_bookmarks/folder_node.html File chrome/browser/resources/md_bookmarks/folder_node.html ...
7 years, 3 months ago (2016-12-08 02:37:58 UTC) #13
angelayang
Hello Cleaned things up, thankyou https://codereview.appspot.com/318910043/diff/130002/chrome/browser/browser_resources.grd File chrome/browser/browser_resources.grd (right): https://codereview.appspot.com/318910043/diff/130002/chrome/browser/browser_resources.grd#newcode256 chrome/browser/browser_resources.grd:256: <include name="IDR_MD_BOOKMARKS_FOLDER_NODE_JS" file="resources\md_bookmarks\folder_node.js" type="BINDATA" ...
7 years, 3 months ago (2016-12-08 02:58:23 UTC) #14
calamity
lgtm. https://codereview.appspot.com/318910043/diff/150001/chrome/browser/resources/md_bookmarks/shared_vars.html File chrome/browser/resources/md_bookmarks/shared_vars.html (right): https://codereview.appspot.com/318910043/diff/150001/chrome/browser/resources/md_bookmarks/shared_vars.html#newcode3 chrome/browser/resources/md_bookmarks/shared_vars.html:3: --active-folder-color: #4285F4; Missed an f.
7 years, 3 months ago (2016-12-08 03:23:00 UTC) #15
angelayang
7 years, 3 months ago (2016-12-08 05:04:21 UTC) #16
Message was sent while issue was closed.
Committed patchset #10 (id:150001) manually as
9237b888171bc235ca023b841ea7bedfe3895b54.
Sign in to reply to this message.

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