chrome/browser/resources/md_bookmarks/command_manager.html - Issue 2834493006: MD Bookmarks: Pull context menu into separate element

Side by Side Diff: chrome/browser/resources/md_bookmarks/command_manager.html

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

Patch Set: Further simplify commands Created 3 years, 8 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

« no previous file with comments | « chrome/browser/resources/md_bookmarks/app.html ('k') | chrome/browser/resources/md_bookmarks/command_manager.js » ('j') | chrome/browser/resources/md_bookmarks/command_manager.js » ('J')
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

OLD	NEW
(Empty)
	1 <link rel="import" href="chrome://resources/html/polymer.html">

	2 <link rel="import" href="chrome://resources/cr_elements/cr_action_menu/cr_action _menu.html">

	3 <link rel="import" href="chrome://resources/cr_elements/cr_lazy_render/cr_lazy_r ender.html">

	4 <link rel="import" href="chrome://bookmarks/edit_dialog.html">

	5 <link rel="import" href="chrome://bookmarks/shared_style.html">

	6 <link rel="import" href="chrome://bookmarks/store_client.html">

	7

	8 <dom-module id="bookmarks-command-manager">

	9 <template>

	10 <style include="shared-style">

	11 </style>
	calamity 2017/04/24 05:20:35 nit: Unnecessary? nit: Unnecessary? tsergeant 2017/04/24 07:28:23 Oops, done. Show quoted text On 2017/04/24 05:20:35, calamity wrote: > nit: Unnecessary? Oops, done.
	12 <dialog is="cr-action-menu" id="dropdown" on-mousedown="onMenuMousedown_">

	13 <button class="dropdown-item"

	14 command="edit"

	15 hidden$="[[!canEdit(menuIds_)]]"

	16 on-tap="onCommandClick_">

	17 [[getEditActionLabel_(menuIds_)]]

	18 </button>

	19 <button class="dropdown-item"

	20 command="copy"

	21 hidden$="[[!canCopy(menuIds_)]]"
	calamity 2017/04/24 05:20:35 I'm conflicted about whether I want this to be can I'm conflicted about whether I want this to be canCopy or canExecute('copy'). When keyboard shortcuts come in, it'll need to be able to ask if certain things are do-able, so it might be nicer to have something generic? Dunno, we can cross that bridge when we get to it I suppose. tsergeant 2017/04/24 07:28:23 I actually used to have it like canExecute('copy') Show quoted text On 2017/04/24 05:20:35, calamity wrote: > I'm conflicted about whether I want this to be canCopy or canExecute('copy'). > When keyboard shortcuts come in, it'll need to be able to ask if certain things > are do-able, so it might be nicer to have something generic? Dunno, we can cross > that bridge when we get to it I suppose. I actually used to have it like canExecute('copy'). I'm not too attached to either method, but I think I find this one nicer to read. My intention for keyboard shortcuts is that there'll be a list of registered commands with their canExecute/handle functions which we can use to figure that stuff out. I've sent you a short design doc I was working on for this -- feel free to make suggestions about the design there. calamity 2017/04/26 03:43:44 Yeah, just saw it this morning. Sorry. Show quoted text On 2017/04/24 07:28:23, tsergeant wrote: > On 2017/04/24 05:20:35, calamity wrote: > > I'm conflicted about whether I want this to be canCopy or canExecute('copy'). > > When keyboard shortcuts come in, it'll need to be able to ask if certain > things > > are do-able, so it might be nicer to have something generic? Dunno, we can > cross > > that bridge when we get to it I suppose. > > I actually used to have it like canExecute('copy'). I'm not too attached to > either method, but I think I find this one nicer to read. > > My intention for keyboard shortcuts is that there'll be a list of registered > commands with their canExecute/handle functions which we can use to figure that > stuff out. > > I've sent you a short design doc I was working on for this -- feel free to make > suggestions about the design there. Yeah, just saw it this morning. Sorry.
	22 on-tap="onCommandClick_">

	23 $i18n{menuCopyURL}

	24 </button>

	25 <button class="dropdown-item"

	26 command="delete"

	27 hidden$="[[!canDelete(menuIds_)]]"

	28 on-tap="onCommandClick_">

	29 $i18n{menuDelete}

	30 </button>
	calamity 2017/04/24 05:20:35 I mean, looking at this, there's some part of me t I mean, looking at this, there's some part of me that wants to believe in a dom-repeat solution, where given a bunch of commands to put in the menu, the dropdown menu would generate and correctly figure out what to hide and show at any given time. This isn't terrible though, and is probably much simpler. tsergeant 2017/04/24 07:28:23 The biggest thing that gets in the way of the dom- Show quoted text On 2017/04/24 05:20:35, calamity wrote: > I mean, looking at this, there's some part of me that wants to believe in a > dom-repeat solution, where given a bunch of commands to put in the menu, the > dropdown menu would generate and correctly figure out what to hide and show at > any given time. > > This isn't terrible though, and is probably much simpler. The biggest thing that gets in the way of the dom-repeat solution is that we're going to have a <hr> separator in the middle of the context menu. Doing it this way is much simpler, even though it's pretty verbose. calamity 2017/04/26 03:43:44 Fair enough. Show quoted text On 2017/04/24 07:28:23, tsergeant wrote: > On 2017/04/24 05:20:35, calamity wrote: > > I mean, looking at this, there's some part of me that wants to believe in a > > dom-repeat solution, where given a bunch of commands to put in the menu, the > > dropdown menu would generate and correctly figure out what to hide and show at > > any given time. > > > > This isn't terrible though, and is probably much simpler. > > The biggest thing that gets in the way of the dom-repeat solution is that we're > going to have a <hr> separator in the middle of the context menu. Doing it this > way is much simpler, even though it's pretty verbose. Fair enough.
	31 </dialog>

	32 <template is="cr-lazy-render" id="editDialog">

	33 <bookmarks-edit-dialog></bookmarks-edit-dialog>

	34 </template>

	35 </template>

	36 <script src="chrome://bookmarks/command_manager.js"></script>

	37 </dom-module>

OLD	NEW