Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(9)

Issue 2834493006: MD Bookmarks: Pull context menu into separate element (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 months ago by tsergeant
Modified:
1 month, 2 weeks ago
Reviewers:
calamity
CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org, michaelpg+watch-md-ui_chromium.org, vitalyp+closure_chromium.org, jlklein+watch-closure_chromium.org, arv+watch_chromium.org, dbeam+watch-closure_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

MD Bookmarks: Pull context menu into separate element This creates a new element, <bookmarks-command-manager>, which is responsible for showing the context menu, hiding commands which are unavailable and responding to command clicks. Pulling out the context menu like this will make it easier to reuse in multiple places, and make it simpler to add keyboard shortcuts for menu items. BUG=692827 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2834493006 Cr-Commit-Position: refs/heads/master@{#469592} Committed: https://chromium.googlesource.com/chromium/src/+/77365188782303a17f15571a0f253dcb5cc0e0b9

Patch Set 1 #

Patch Set 2 : Simplify #

Patch Set 3 : Simplify #

Patch Set 4 : Further simplify commands #

Total comments: 16

Patch Set 5 : Review comments #

Patch Set 6 : Switch back to using a Command enum #

Total comments: 2

Patch Set 7 : Rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+295 lines, -103 lines) Patch
M chrome/browser/browser_resources.grd View 1 2 3 4 5 6 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/resources/md_bookmarks/app.html View 1 2 3 4 5 6 2 chunks +2 lines, -0 lines 0 comments Download
A chrome/browser/resources/md_bookmarks/command_manager.html View 1 2 3 4 5 1 chunk +34 lines, -0 lines 0 comments Download
A chrome/browser/resources/md_bookmarks/command_manager.js View 1 2 3 4 5 1 chunk +168 lines, -0 lines 0 comments Download
M chrome/browser/resources/md_bookmarks/compiled_resources2.gyp View 1 2 3 4 5 6 3 chunks +14 lines, -4 lines 0 comments Download
M chrome/browser/resources/md_bookmarks/constants.js View 5 1 chunk +10 lines, -0 lines 0 comments Download
M chrome/browser/resources/md_bookmarks/item.js View 1 2 3 4 5 6 2 chunks +0 lines, -2 lines 0 comments Download
M chrome/browser/resources/md_bookmarks/list.html View 1 2 3 4 5 6 3 chunks +0 lines, -18 lines 0 comments Download
M chrome/browser/resources/md_bookmarks/list.js View 1 2 3 4 5 6 3 chunks +0 lines, -79 lines 0 comments Download
M chrome/browser/ui/webui/md_bookmarks/md_bookmarks_ui.cc View 1 2 3 4 5 6 1 chunk +4 lines, -0 lines 0 comments Download
A chrome/test/data/webui/md_bookmarks/command_manager_test.js View 1 2 3 4 5 1 chunk +47 lines, -0 lines 0 comments Download
M chrome/test/data/webui/md_bookmarks/md_bookmarks_browsertest.js View 1 2 3 4 5 6 1 chunk +14 lines, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Dependent Patchsets:

Messages

Total messages: 38 (28 generated)
tsergeant
2 months ago (2017-04-21 06:33:34 UTC) #7
calamity
https://codereview.chromium.org/2834493006/diff/60001/chrome/browser/resources/md_bookmarks/command_manager.html File chrome/browser/resources/md_bookmarks/command_manager.html (right): https://codereview.chromium.org/2834493006/diff/60001/chrome/browser/resources/md_bookmarks/command_manager.html#newcode11 chrome/browser/resources/md_bookmarks/command_manager.html:11: </style> nit: Unnecessary? https://codereview.chromium.org/2834493006/diff/60001/chrome/browser/resources/md_bookmarks/command_manager.html#newcode21 chrome/browser/resources/md_bookmarks/command_manager.html:21: hidden$="[[!canCopy(menuIds_)]]" I'm conflicted about ...
2 months ago (2017-04-24 05:20:35 UTC) #12
tsergeant
https://codereview.chromium.org/2834493006/diff/60001/chrome/browser/resources/md_bookmarks/command_manager.html File chrome/browser/resources/md_bookmarks/command_manager.html (right): https://codereview.chromium.org/2834493006/diff/60001/chrome/browser/resources/md_bookmarks/command_manager.html#newcode11 chrome/browser/resources/md_bookmarks/command_manager.html:11: </style> On 2017/04/24 05:20:35, calamity wrote: > nit: Unnecessary? ...
2 months ago (2017-04-24 07:28:23 UTC) #17
calamity
lgtm. https://codereview.chromium.org/2834493006/diff/60001/chrome/browser/resources/md_bookmarks/command_manager.html File chrome/browser/resources/md_bookmarks/command_manager.html (right): https://codereview.chromium.org/2834493006/diff/60001/chrome/browser/resources/md_bookmarks/command_manager.html#newcode21 chrome/browser/resources/md_bookmarks/command_manager.html:21: hidden$="[[!canCopy(menuIds_)]]" On 2017/04/24 07:28:23, tsergeant wrote: > On ...
1 month, 4 weeks ago (2017-04-26 03:43:44 UTC) #20
tsergeant
I think you're right: using common functions and a Command enum seems to be the ...
1 month, 4 weeks ago (2017-04-26 05:49:04 UTC) #21
calamity
https://codereview.chromium.org/2834493006/diff/100001/chrome/browser/resources/md_bookmarks/command_manager.js File chrome/browser/resources/md_bookmarks/command_manager.js (right): https://codereview.chromium.org/2834493006/diff/100001/chrome/browser/resources/md_bookmarks/command_manager.js#newcode68 chrome/browser/resources/md_bookmarks/command_manager.js:68: return !!node.url; Wait, I should be able to copy ...
1 month, 3 weeks ago (2017-04-27 03:42:04 UTC) #26
tsergeant
https://codereview.chromium.org/2834493006/diff/100001/chrome/browser/resources/md_bookmarks/command_manager.js File chrome/browser/resources/md_bookmarks/command_manager.js (right): https://codereview.chromium.org/2834493006/diff/100001/chrome/browser/resources/md_bookmarks/command_manager.js#newcode68 chrome/browser/resources/md_bookmarks/command_manager.js:68: return !!node.url; On 2017/04/27 03:42:04, calamity wrote: > Wait, ...
1 month, 3 weeks ago (2017-04-27 04:08:37 UTC) #27
calamity
On 2017/04/27 04:08:37, tsergeant wrote: > https://codereview.chromium.org/2834493006/diff/100001/chrome/browser/resources/md_bookmarks/command_manager.js > File chrome/browser/resources/md_bookmarks/command_manager.js (right): > > https://codereview.chromium.org/2834493006/diff/100001/chrome/browser/resources/md_bookmarks/command_manager.js#newcode68 > ...
1 month, 3 weeks ago (2017-04-28 04:04:46 UTC) #28
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2834493006/120001
1 month, 2 weeks ago (2017-05-05 03:02:17 UTC) #35
commit-bot: I haz the power
1 month, 2 weeks ago (2017-05-05 04:04:02 UTC) #38
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/77365188782303a17f15571a0f25...
Sign in to reply to this message.

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