Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2814743007:
[cr-action-menu] Allow configurable anchors. (Closed)
|
Created:
3 years, 8 months ago by calamity Modified:
3 years, 7 months ago CC:
chrome-apps-syd-reviews_chromium.org, chromium-reviews, dbeam+watch-elements_chromium.org, michaelpg+watch-elements_chromium.org, oshima+watch_chromium.org, stevenjb+watch-md-settings_chromium.org, tsergeant Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[cr-action-menu] Allow configurable anchors.
This CL adds showAtPosition to cr-action-menu which allows a client to
specify a rect or point to anchor to and which side of the menu to align.
This will be used in MD Bookmarks for right click context menus which
require more flexible alignment.
BUG=692837
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2814743007
Cr-Commit-Position: refs/heads/master@{#469257}
Committed: https://chromium.googlesource.com/chromium/src/+/3693ae6f5653c20d845aae55fb2b734b46c337a8
Patch Set 1 : #
Total comments: 12
Patch Set 2 : rebase, address comments #
Total comments: 14
Patch Set 3 : address_comments #Patch Set 4 : address comment #
Total comments: 2
Patch Set 5 : use AnchorAlignment #
Total comments: 6
Patch Set 6 : rebase, address comments #Patch Set 7 : rebase #
Messages
Total messages: 42 (22 generated)
Description was changed from ========== [cr-action-menu] Allow configurable anchors. This CL adds showAtPosition to cr-action-menu which allows a client to specify a rect or point to anchor to and which side of the menu to align. BUG=692837 ========== to ========== [cr-action-menu] Allow configurable anchors. This CL adds showAtPosition to cr-action-menu which allows a client to specify a rect or point to anchor to and which side of the menu to align. BUG=692837 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
calamity@chromium.org changed reviewers: + dpapad@chromium.org
Description was changed from ========== [cr-action-menu] Allow configurable anchors. This CL adds showAtPosition to cr-action-menu which allows a client to specify a rect or point to anchor to and which side of the menu to align. BUG=692837 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [cr-action-menu] Allow configurable anchors. This CL adds showAtPosition to cr-action-menu which allows a client to specify a rect or point to anchor to and which side of the menu to align. This will be used in MD Bookmarks for right click context menus which require more flexible alignment. BUG=692837 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
https://codereview.chromium.org/2814743007/diff/40001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js (left): https://codereview.chromium.org/2814743007/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:187: // Attempt to show the menu starting from the top of the rectangle and Has this logic been preserved in the new code? It is not obvious, can you point out where this is happening and also preserve this comment? https://codereview.chromium.org/2814743007/diff/40001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js (right): https://codereview.chromium.org/2814743007/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:173: anchorPositionX: -1, Please explain this with a comment. https://codereview.chromium.org/2814743007/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:182: * bottom: (number| undefined), Where is config.bottom and config.right used? https://codereview.chromium.org/2814743007/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:191: */ Can we make a typedef for this? Such that clients of showAtPosition() don't need to repeat this entire type when they document their code? https://codereview.chromium.org/2814743007/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:199: function(value, defaultValue) { Can fit in previous line? https://codereview.chromium.org/2814743007/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:228: function getStartPointWithAnchor( @param annotations missing for this function. Also can we move it outside of showAtPosition? showAtPosition is now fairly long, anything to make it shorter will help readability.
tsergeant@chromium.org changed reviewers: + tsergeant@chromium.org
Looks like you need to rebase past https://codereview.chromium.org/2810873002, which causes an error when your code is patched on top of it.
On 2017/04/13 at 01:08:15, tsergeant wrote: > Looks like you need to rebase past https://codereview.chromium.org/2810873002, which causes an error when your code is patched on top of it. Also, since branch cut M59 is tomorrow, suggesting to not land this before the branch cut, to decrease the possibility of causing any regressions at the last moment.
tsergeant@chromium.org changed reviewers: - tsergeant@chromium.org
Patchset #2 (id:60001) has been deleted
https://codereview.chromium.org/2814743007/diff/40001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js (left): https://codereview.chromium.org/2814743007/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:187: // Attempt to show the menu starting from the top of the rectangle and On 2017/04/12 17:12:32, dpapad wrote: > Has this logic been preserved in the new code? It is not obvious, can you point > out where this is happening and also preserve this comment? Yes, this behavior is what getStartPointWithAnchor genericizes. This comment has somewhat been added to the getStartPointWithAnchor method. https://codereview.chromium.org/2814743007/diff/40001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js (right): https://codereview.chromium.org/2814743007/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:173: anchorPositionX: -1, On 2017/04/12 17:12:32, dpapad wrote: > Please explain this with a comment. Done. https://codereview.chromium.org/2814743007/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:182: * bottom: (number| undefined), On 2017/04/12 17:12:32, dpapad wrote: > Where is config.bottom and config.right used? Oops, these were supposed to change to width/height. Fixed. https://codereview.chromium.org/2814743007/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:191: */ On 2017/04/12 17:12:32, dpapad wrote: > Can we make a typedef for this? Such that clients of showAtPosition() don't need > to repeat this entire type when they document their code? Done. https://codereview.chromium.org/2814743007/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:199: function(value, defaultValue) { On 2017/04/12 17:12:32, dpapad wrote: > Can fit in previous line? Done. https://codereview.chromium.org/2814743007/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:228: function getStartPointWithAnchor( On 2017/04/12 17:12:32, dpapad wrote: > @param annotations missing for this function. Also can we move it outside of > showAtPosition? showAtPosition is now fairly long, anything to make it shorter > will help readability. Done.
https://codereview.chromium.org/2814743007/diff/80001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js (right): https://codereview.chromium.org/2814743007/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:33: * where focus will be returned after the menu is closed. This needs a comment now, that says "Only populated if action menu is opened with showAt()" https://codereview.chromium.org/2814743007/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:173: this.anchorElement_ = null; This line should be inside the if too, if (this.anchorElement_) { this.anchorElement_.focus(); this.anchorElement_ = null; } https://codereview.chromium.org/2814743007/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:209: var startPoint = (start + end - length) / 2 + TL;DR equivalent to var startPoint = anchorPosition == 1 ? start : end - length; This is an overly complex calculation I think. Can be simplified as follows: 1) Solving the equation, a little bit further leads to (using aP for anchorPosition) var startPoint = (start * (1 + aP) + (1 - aP) * (end - length)) / 2; 2) Now, I believe that anchorPosition can only be 1 or -1. So: - if aP = -1: var startPoint = end - length; // Much simpler! - if aP = 1: var = start; // Much much simpler!! So to sum up, I think this code would be written simpler as follows assert(Math.abs(anchorPosition) == 1); var startPoint = anchorPosition == 1 ? start : end - length; https://codereview.chromium.org/2814743007/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:220: * @param {ShowConfig} config !ShowConfig https://codereview.chromium.org/2814743007/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:232: var top = config.top; Lines 232-241 are basically creating a ShowConfig that has all optional values populated. Can you package this functionality avoiding flattening? How about the following? Make a helper method |normalizeConfig| that takes the input config object and returns the fully populated ShowConfig object. var effectiveConfig = normalizeConfig(config); Then use |effectiveConfig| for all subsequent calculations, no need to flatten all 10 variables.
Patchset #3 (id:100001) has been deleted
https://codereview.chromium.org/2814743007/diff/80001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js (right): https://codereview.chromium.org/2814743007/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:33: * where focus will be returned after the menu is closed. On 2017/04/14 22:06:31, dpapad wrote: > This needs a comment now, that says "Only populated if action menu is opened > with showAt()" Done. https://codereview.chromium.org/2814743007/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:173: this.anchorElement_ = null; On 2017/04/14 22:06:31, dpapad wrote: > This line should be inside the if too, > > if (this.anchorElement_) { > this.anchorElement_.focus(); > this.anchorElement_ = null; > } Done. https://codereview.chromium.org/2814743007/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:209: var startPoint = (start + end - length) / 2 + On 2017/04/14 22:06:31, dpapad wrote: > TL;DR equivalent to > var startPoint = anchorPosition == 1 ? start : end - length; > > This is an overly complex calculation I think. Can be simplified as follows: > > 1) Solving the equation, a little bit further leads to (using aP for > anchorPosition) > var startPoint = (start * (1 + aP) + (1 - aP) * (end - length)) / 2; > > 2) Now, I believe that anchorPosition can only be 1 or -1. So: > - if aP = -1: > var startPoint = end - length; // Much simpler! > - if aP = 1: > var = start; // Much much simpler!! > > So to sum up, I think this code would be written simpler as follows > assert(Math.abs(anchorPosition) == 1); > var startPoint = anchorPosition == 1 ? start : end - length; It depends if you ever want to center-align a menu to a rect (or even do a percentage offset) by using anchorPosition = 0/0.5/etc (there's a test case for this). I think being able to anchor in the general case is a good generalization to have for this kind of library element. Center-aligning may have uses such as when implementing a dropdown, for example. https://codereview.chromium.org/2814743007/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:220: * @param {ShowConfig} config On 2017/04/14 22:06:31, dpapad wrote: > !ShowConfig Done. https://codereview.chromium.org/2814743007/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:232: var top = config.top; On 2017/04/14 22:06:31, dpapad wrote: > Lines 232-241 are basically creating a ShowConfig that has all optional values > populated. Can you package this functionality avoiding flattening? How about the > following? > > Make a helper method |normalizeConfig| that takes the input config object and > returns the fully populated ShowConfig object. > > var effectiveConfig = normalizeConfig(config); > Then use |effectiveConfig| for all subsequent calculations, no need to flatten > all 10 variables. Done.
https://codereview.chromium.org/2814743007/diff/80001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js (right): https://codereview.chromium.org/2814743007/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:209: var startPoint = (start + end - length) / 2 + > It depends if you ever want to center-align a menu to a rect (or even do a percentage offset) by using anchorPosition = 0/0.5/etc (there's a test case for this). > > I think being able to anchor in the general case is a good generalization to have for this kind of library element. Center-aligning may have uses such as when implementing a dropdown, for example. Ok, I kind of understand the generalization need better now. But I am also realizing that |anchorPosition| semantics are under-documented as it stands. 1) The "position" suffix makes it sound as if it holds px values. Can we change the suffix from "position" to "percent"? 2) anchorPosition is a percent of which value, "end - start", "(end - start) / 2"? Needs to be explicitly stated. 3) Anchor position is basically an offset percentage. - What is the starting point where the offset is applied at? It took me a bit to figure out that it is the mid point "(end-start)/2". - Can we rename |anchorPosition| to |anchorOffsetPercent|? - Can we provide examples? Specifically, see http://imgur.com/a/HWhcw for the cases I had in mind (which X,Y combination achieves each case).
https://codereview.chromium.org/2814743007/diff/80001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js (right): https://codereview.chromium.org/2814743007/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:209: var startPoint = (start + end - length) / 2 + On 2017/04/19 21:41:55, dpapad wrote: > > It depends if you ever want to center-align a menu to a rect (or even do a > percentage offset) by using anchorPosition = 0/0.5/etc (there's a test case for > this). > > > > I think being able to anchor in the general case is a good generalization to > have for this kind of library element. Center-aligning may have uses such as > when implementing a dropdown, for example. > > Ok, I kind of understand the generalization need better now. But I am also > realizing that |anchorPosition| semantics are under-documented as it stands. > > 1) The "position" suffix makes it sound as if it holds px values. Can we change > the suffix from "position" to "percent"? > 2) anchorPosition is a percent of which value, "end - start", "(end - start) / > 2"? Needs to be explicitly stated. > 3) Anchor position is basically an offset percentage. > - What is the starting point where the offset is applied at? It took me a bit > to figure out that it is the mid point "(end-start)/2". > - Can we rename |anchorPosition| to |anchorOffsetPercent|? > - Can we provide examples? Specifically, see http://imgur.com/a/HWhcw for the > cases I had in mind (which X,Y combination achieves each case). I'm not a fan of calling this percent, given that it's not out of 100. I've renamed it to anchorConfig so that it's obvious that more reading is required. I've added a description of how to achieve certain alignments using this API. Another option, if you prefer, is to have an enum: enum AnchorAlignment { START_OUTSIDE, START_INSIDE, CENTER, END_INSIDE, END_OUTSIDE, } which would cover all cases and be more understandable.
https://codereview.chromium.org/2814743007/diff/80001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js (right): https://codereview.chromium.org/2814743007/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:209: var startPoint = (start + end - length) / 2 + On 2017/04/24 at 05:20:54, calamity wrote: > On 2017/04/19 21:41:55, dpapad wrote: > > > It depends if you ever want to center-align a menu to a rect (or even do a > > percentage offset) by using anchorPosition = 0/0.5/etc (there's a test case for > > this). > > > > > > I think being able to anchor in the general case is a good generalization to > > have for this kind of library element. Center-aligning may have uses such as > > when implementing a dropdown, for example. > > > > Ok, I kind of understand the generalization need better now. But I am also > > realizing that |anchorPosition| semantics are under-documented as it stands. > > > > 1) The "position" suffix makes it sound as if it holds px values. Can we change > > the suffix from "position" to "percent"? > > 2) anchorPosition is a percent of which value, "end - start", "(end - start) / > > 2"? Needs to be explicitly stated. > > 3) Anchor position is basically an offset percentage. > > - What is the starting point where the offset is applied at? It took me a bit > > to figure out that it is the mid point "(end-start)/2". > > - Can we rename |anchorPosition| to |anchorOffsetPercent|? > > - Can we provide examples? Specifically, see http://imgur.com/a/HWhcw for the > > cases I had in mind (which X,Y combination achieves each case). > > I'm not a fan of calling this percent, given that it's not out of 100. I've renamed it to anchorConfig so that it's obvious that more reading is required. > > I've added a description of how to achieve certain alignments using this API. > > Another option, if you prefer, is to have an enum: > > enum AnchorAlignment { > START_OUTSIDE, > START_INSIDE, > CENTER, > END_INSIDE, > END_OUTSIDE, > } > > which would cover all cases and be more understandable. If you think that will be cleaner, I am fine with it. I am not 100% sure how the _INSIDE and _OUTSIDE cases can be differentiated though using a single enum. The graph pasted above is how I currently understand the |anchorConfig| concept. Regarding percent VS config, I realize now that both are not great names. Anyway, I am not so worried about the names, if the code has enough explanations (FWIW, anchorOffsetFactor might be more clear). https://codereview.chromium.org/2814743007/diff/140001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js (right): https://codereview.chromium.org/2814743007/diff/140001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:203: * the anchor would use a config of (1, -1). This explanation is much better than before. Can we include an ASCII graph though to make it even better? For example: x indicates the center point of the menu to be displayed. (anchorConfigX, anchorConfigY) indicates the relative coordinates to the anchor box. (-1, -1) (0, -1) (1, -1) x_____x_____x | | | | | (0, 0) | (-1, 0) x x x (1, 0) | | |anchor box | |_____ _____| x x x (-1, 1) (0, 1) (1, 1)
https://codereview.chromium.org/2814743007/diff/80001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js (right): https://codereview.chromium.org/2814743007/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:209: var startPoint = (start + end - length) / 2 + On 2017/04/25 17:42:43, dpapad wrote: > On 2017/04/24 at 05:20:54, calamity wrote: > > On 2017/04/19 21:41:55, dpapad wrote: > > > > It depends if you ever want to center-align a menu to a rect (or even do > a > > > percentage offset) by using anchorPosition = 0/0.5/etc (there's a test case > for > > > this). > > > > > > > > I think being able to anchor in the general case is a good generalization > to > > > have for this kind of library element. Center-aligning may have uses such as > > > when implementing a dropdown, for example. > > > > > > Ok, I kind of understand the generalization need better now. But I am also > > > realizing that |anchorPosition| semantics are under-documented as it stands. > > > > > > 1) The "position" suffix makes it sound as if it holds px values. Can we > change > > > the suffix from "position" to "percent"? > > > 2) anchorPosition is a percent of which value, "end - start", "(end - start) > / > > > 2"? Needs to be explicitly stated. > > > 3) Anchor position is basically an offset percentage. > > > - What is the starting point where the offset is applied at? It took me a > bit > > > to figure out that it is the mid point "(end-start)/2". > > > - Can we rename |anchorPosition| to |anchorOffsetPercent|? > > > - Can we provide examples? Specifically, see http://imgur.com/a/HWhcw for > the > > > cases I had in mind (which X,Y combination achieves each case). > > > > I'm not a fan of calling this percent, given that it's not out of 100. I've > renamed it to anchorConfig so that it's obvious that more reading is required. > > > > I've added a description of how to achieve certain alignments using this API. > > > > Another option, if you prefer, is to have an enum: > > > > enum AnchorAlignment { > > START_OUTSIDE, > > START_INSIDE, > > CENTER, > > END_INSIDE, > > END_OUTSIDE, > > } > > > > which would cover all cases and be more understandable. > > If you think that will be cleaner, I am fine with it. I am not 100% sure how the > _INSIDE and _OUTSIDE cases can be differentiated though using a single enum. The > graph pasted above is how I currently understand the |anchorConfig| concept. > > Regarding percent VS config, I realize now that both are not great names. > Anyway, I am not so worried about the names, if the code has enough explanations > (FWIW, anchorOffsetFactor might be more clear). Changed to AnchorAlignment. I think this covers likely use cases and is easy to understand. https://codereview.chromium.org/2814743007/diff/140001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js (right): https://codereview.chromium.org/2814743007/diff/140001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:203: * the anchor would use a config of (1, -1). On 2017/04/25 17:42:43, dpapad wrote: > This explanation is much better than before. Can we include an ASCII graph > though to make it even better? For example: > > x indicates the center point of the menu to be displayed. > (anchorConfigX, anchorConfigY) indicates the relative coordinates to the anchor > box. > > > (-1, -1) (0, -1) (1, -1) > x_____x_____x > | | > | | > | (0, 0) | > (-1, 0) x x x (1, 0) > | | > |anchor box | > |_____ _____| > x x x > (-1, 1) (0, 1) (1, 1) Done.
https://codereview.chromium.org/2814743007/diff/160001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js (right): https://codereview.chromium.org/2814743007/diff/160001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:22: * @enum @enum {number} https://codereview.chromium.org/2814743007/diff/160001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:203: anchorAlignmentX: -1, Should we be using the AnchorAlignment enum now that we have one? https://codereview.chromium.org/2814743007/diff/160001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:322: getDefaultShowConfig_: function() { This method (and also getStartPointWithAnchor_ above) does not need to be a method on |this|, it can simply be a helper function, private to this file. Perhaps use a function() { ... }(); wrapper around the entire file, to ensure we don't leak unnecessarily functions to the global scope?
https://codereview.chromium.org/2814743007/diff/160001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js (right): https://codereview.chromium.org/2814743007/diff/160001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:22: * @enum On 2017/04/27 01:30:23, dpapad wrote: > @enum {number} Done. https://codereview.chromium.org/2814743007/diff/160001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:203: anchorAlignmentX: -1, On 2017/04/27 01:30:23, dpapad wrote: > Should we be using the AnchorAlignment enum now that we have one? Done. https://codereview.chromium.org/2814743007/diff/160001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:322: getDefaultShowConfig_: function() { On 2017/04/27 01:30:23, dpapad wrote: > This method (and also getStartPointWithAnchor_ above) does not need to be a > method on |this|, it can simply be a helper function, private to this file. > Perhaps use a function() { ... }(); wrapper around the entire file, to ensure we > don't leak unnecessarily functions to the global scope? Done.
dpapad@chromium.org changed reviewers: + dbeam@chromium.org
LGTM! +dbeam Having said that, we (MD Settings) might need to make some changes in cr-action-menu for the purposes of addressing https://bugs.chromium.org/p/chromium/issues/detail?id=716184, and possibly merge those changes to M59. Perhaps we should wait before landing this CL, otherwise a merge might be out of the question (if we endup having to merge this CL too). @dbeam WDYT?
On 2017/04/27 21:38:36, dpapad wrote: > LGTM! > > +dbeam > > Having said that, we (MD Settings) might need to make some changes in > cr-action-menu for the purposes of > addressing https://bugs.chromium.org/p/chromium/issues/detail?id=716184, and > possibly merge those changes to M59. > Perhaps we should wait before landing this CL, otherwise a merge might be out of > the question (if we endup > having to merge this CL too). @dbeam WDYT? Understood. However, this is blocking a few other in-flight CLs, so it would be good to resolve that issue quickly. Is there any timeline for when that issue will get fixed?
On 2017/04/28 04:22:30, calamity wrote: > On 2017/04/27 21:38:36, dpapad wrote: > > LGTM! > > > > +dbeam > > > > Having said that, we (MD Settings) might need to make some changes in > > cr-action-menu for the purposes of > > addressing https://bugs.chromium.org/p/chromium/issues/detail?id=716184, and > > possibly merge those changes to M59. > > Perhaps we should wait before landing this CL, otherwise a merge might be out > of > > the question (if we endup > > having to merge this CL too). @dbeam WDYT? > > Understood. However, this is blocking a few other in-flight CLs, so it would be > good to resolve that issue quickly. Is there any timeline for when that issue > will get fixed? https://codereview.chromium.org/2842303004/
The CQ bit was checked by calamity@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hey, now that the ink patch has landed, are you fine for me to land this?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
cool with me!
The CQ bit was checked by tsergeant@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by calamity@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org Link to the patchset: https://codereview.chromium.org/2814743007/#ps200001 (title: "rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1493867323183530,
"parent_rev": "8d47055fe57c19eee560dfd25f5109845b1ad7fc", "commit_rev":
"3693ae6f5653c20d845aae55fb2b734b46c337a8"}
Message was sent while issue was closed.
Description was changed from ========== [cr-action-menu] Allow configurable anchors. This CL adds showAtPosition to cr-action-menu which allows a client to specify a rect or point to anchor to and which side of the menu to align. This will be used in MD Bookmarks for right click context menus which require more flexible alignment. BUG=692837 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [cr-action-menu] Allow configurable anchors. This CL adds showAtPosition to cr-action-menu which allows a client to specify a rect or point to anchor to and which side of the menu to align. This will be used in MD Bookmarks for right click context menus which require more flexible alignment. BUG=692837 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2814743007 Cr-Commit-Position: refs/heads/master@{#469257} Committed: https://chromium.googlesource.com/chromium/src/+/3693ae6f5653c20d845aae55fb2b... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:200001) as https://chromium.googlesource.com/chromium/src/+/3693ae6f5653c20d845aae55fb2b... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
