Chromium Code Reviews
Help | Chromium Project | Sign in
(2)

Issue 2834493006: MD Bookmarks: Pull context menu into separate element

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 week, 1 day ago by tsergeant
Modified:
1 day, 11 hours 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

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
Unified diffs Side-by-side diffs Delta from patch set Stats (+295 lines, -104 lines) Patch
M chrome/browser/browser_resources.grd View 1 2 3 4 5 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/resources/md_bookmarks/app.html View 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 2 comments Download
M chrome/browser/resources/md_bookmarks/compiled_resources2.gyp View 4 chunks +14 lines, -5 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 chunks +0 lines, -2 lines 0 comments Download
M chrome/browser/resources/md_bookmarks/list.html View 3 chunks +0 lines, -18 lines 0 comments Download
M chrome/browser/resources/md_bookmarks/list.js View 3 chunks +0 lines, -79 lines 0 comments Download
M chrome/browser/ui/webui/md_bookmarks/md_bookmarks_ui.cc View 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 1 chunk +14 lines, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Depends on Patchset:

Dependent Patchsets:

Messages

Total messages: 28 (20 generated)
tsergeant
1 week, 1 day 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 ...
5 days, 10 hours 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? ...
5 days, 7 hours 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 ...
3 days, 11 hours 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 ...
3 days, 9 hours 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 ...
2 days, 11 hours 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, ...
2 days, 11 hours ago (2017-04-27 04:08:37 UTC) #27
calamity
1 day, 11 hours ago (2017-04-28 04:04:46 UTC) #28
On 2017/04/27 04:08:37, tsergeant wrote:
>
https://codereview.chromium.org/2834493006/diff/100001/chrome/browser/resourc...
> File chrome/browser/resources/md_bookmarks/command_manager.js (right):
> 
>
https://codereview.chromium.org/2834493006/diff/100001/chrome/browser/resourc...
> chrome/browser/resources/md_bookmarks/command_manager.js:68: return
!!node.url;
> On 2017/04/27 03:42:04, calamity wrote:
> > Wait, I should be able to copy multiple items, which may or may not be
> folders,
> > right?.
> 
> You can in the current BMM.
> 
> However, the new spec only shows "Copy URL" as in the menu for items (not for
> folders), and the new string implies that you're only expected to do one at a
> time. I interpreted this to mean we are meant to change the behavior, but I
can
> check with Alan if you'd like.

Yes pls. I don't think we should kill this feature without some explicit
thought. It's not very hard too leave in either.
Sign in to reply to this message.

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