Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1760823002:
Allow splitter to resize next element (Closed)
|
Created:
4 years, 9 months ago by ryoh Modified:
4 years, 9 months ago Reviewers:
Dan Beam CC:
chromium-reviews, oshima+watch_chromium.org, hirono, tapted, Evan Stade Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAllow splitter to resize next element
To specify which element should be resized, I added a new argument "resizeNextElement" to decorate method.
BUG=591611
TEST=none
Committed: https://crrev.com/9613e62a3e9d55ffdc04d1276ee57c54ae8322e2
Cr-Commit-Position: refs/heads/master@{#379737}
Patch Set 1 #
Total comments: 8
Patch Set 2 : use setter instead of argument of decorate, and added tests #
Total comments: 19
Patch Set 3 : updated along the review #
Total comments: 2
Patch Set 4 : remote redundant '+' #Messages
Total messages: 20 (9 generated)
ryoh@chromium.org changed reviewers: + tapted@chromium.org
Hi! I added an option to switch which element should be resized by splitter. Could you review this CL? (I ask you for the review because you reviewed a CL for this file recently: https://codereview.chromium.org/1711233003)
tapted@chromium.org changed reviewers: + estade@chromium.org
estade is a better reviewer for this than me, plus he has the OWNERS bits you will need :)
Description was changed from ========== Allow splitter to resize next element To specify which element should be resized, I added a new argument "resizeNextElement" to decorate method. BUG=591611 TEST=none ========== to ========== Allow splitter to resize next element To specify which element should be resized, I added a new argument "resizeNextElement" to decorate method. BUG=591611 TEST=none ==========
dbeam@chromium.org changed reviewers: + dbeam@chromium.org - estade@chromium.org
I can review for estade@ (I'm sure he's heartbroken, but will recover somehow) please also update chrome/test/data/webui/splitter_test.html https://codereview.chromium.org/1760823002/diff/1/ui/webui/resources/js/cr/ui... File ui/webui/resources/js/cr/ui/splitter.js (right): https://codereview.chromium.org/1760823002/diff/1/ui/webui/resources/js/cr/ui... ui/webui/resources/js/cr/ui/splitter.js:68: * @param {boolean=} resizeNextElement True if resize the next element. this should be named opt_resizeNextElement https://codereview.chromium.org/1760823002/diff/1/ui/webui/resources/js/cr/ui... ui/webui/resources/js/cr/ui/splitter.js:70: (left) element. * @param {boolean=} opt_resizeNextElement ... * ... text continues ... * ... more text. https://codereview.chromium.org/1760823002/diff/1/ui/webui/resources/js/cr/ui... ui/webui/resources/js/cr/ui/splitter.js:77: this.resizeNextElement_ = resizeNextElement || false; this.resizeNextElement_ = !!opt_resizeNextElement; https://codereview.chromium.org/1760823002/diff/1/ui/webui/resources/js/cr/ui... ui/webui/resources/js/cr/ui/splitter.js:213: this.nextElementSibling : this.previousElementSibling; you should make a /** * @return {Element} * @private */ getResizeTarget_: function() { return this.resizeNextElement_ ? this.nextElementSibling : this.previousElementSibling; }, and re-use it from the 3 places you've copy pasted code
tapted@chromium.org changed reviewers: - tapted@chromium.org
Thank you for your review! I found that we can't pass any arguments to decorate function (extra arguments for cr.ui.decorate are just ignored). So, I introduced a setter of "resizeNextElement" instead. I added some tests for Splitter class. Could you take a look? https://codereview.chromium.org/1760823002/diff/1/ui/webui/resources/js/cr/ui... File ui/webui/resources/js/cr/ui/splitter.js (right): https://codereview.chromium.org/1760823002/diff/1/ui/webui/resources/js/cr/ui... ui/webui/resources/js/cr/ui/splitter.js:68: * @param {boolean=} resizeNextElement True if resize the next element. On 2016/03/03 19:32:28, Dan Beam wrote: > this should be named opt_resizeNextElement Done. https://codereview.chromium.org/1760823002/diff/1/ui/webui/resources/js/cr/ui... ui/webui/resources/js/cr/ui/splitter.js:70: (left) element. On 2016/03/03 19:32:28, Dan Beam wrote: > * @param {boolean=} opt_resizeNextElement ... > * ... text continues ... > * ... more text. Done. https://codereview.chromium.org/1760823002/diff/1/ui/webui/resources/js/cr/ui... ui/webui/resources/js/cr/ui/splitter.js:77: this.resizeNextElement_ = resizeNextElement || false; On 2016/03/03 19:32:28, Dan Beam wrote: > this.resizeNextElement_ = !!opt_resizeNextElement; Done. https://codereview.chromium.org/1760823002/diff/1/ui/webui/resources/js/cr/ui... ui/webui/resources/js/cr/ui/splitter.js:213: this.nextElementSibling : this.previousElementSibling; On 2016/03/03 19:32:28, Dan Beam wrote: > you should make a > > /** > * @return {Element} > * @private > */ > getResizeTarget_: function() { > return this.resizeNextElement_ ? this.nextElementSibling : > this.previousElementSibling; > }, > > and re-use it from the 3 places you've copy pasted code Done.
https://codereview.chromium.org/1760823002/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/splitter_test.html (right): https://codereview.chromium.org/1760823002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/splitter_test.html:19: } \n https://codereview.chromium.org/1760823002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/splitter_test.html:35: splitter.dispatchEvent(move); why do we need this middle 'mousemove'? https://codereview.chromium.org/1760823002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/splitter_test.html:46: assertEquals(100, afterWidth-beforeWidth); afterWidth - beforeWidth ^ ^ https://codereview.chromium.org/1760823002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/splitter_test.html:48: function testSplitter_ResizeNextElement() { \n https://codereview.chromium.org/1760823002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/splitter_test.html:62: splitter.dispatchEvent(move); why do we need this middle 'mousemove' as well? https://codereview.chromium.org/1760823002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/splitter_test.html:73: assertEquals(100, afterWidth-beforeWidth); afterWidth - beforeWidth ^ ^ https://codereview.chromium.org/1760823002/diff/20001/ui/webui/resources/js/c... File ui/webui/resources/js/cr/ui/splitter.js (right): https://codereview.chromium.org/1760823002/diff/20001/ui/webui/resources/js/c... ui/webui/resources/js/cr/ui/splitter.js:78: * @param {boolean=} resizeNext True if resize the next element. = means optional. this should not be optional, so I'd remove the = https://codereview.chromium.org/1760823002/diff/20001/ui/webui/resources/js/c... ui/webui/resources/js/cr/ui/splitter.js:82: this.resizeNextElement_ = !!resizeNext; you should only accept booleans (not undefined), so !! should not be needed https://codereview.chromium.org/1760823002/diff/20001/ui/webui/resources/js/c... ui/webui/resources/js/cr/ui/splitter.js:150: * Calculate witdh to resize target element. width https://codereview.chromium.org/1760823002/diff/20001/ui/webui/resources/js/c... ui/webui/resources/js/cr/ui/splitter.js:157: +deltaX; return this.resizeNextElement_ ? -deltaX : deltaX;
Thank you for the review! I updated the patch, could you take another look? https://codereview.chromium.org/1760823002/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/splitter_test.html (right): https://codereview.chromium.org/1760823002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/splitter_test.html:19: } On 2016/03/07 18:14:20, Dan Beam wrote: > \n Done. https://codereview.chromium.org/1760823002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/splitter_test.html:35: splitter.dispatchEvent(move); On 2016/03/07 18:14:19, Dan Beam wrote: > why do we need this middle 'mousemove'? I thought it is more natural to add this middle 'mousemove', because a user may cause more than one 'mousemove' events while resizing element. https://codereview.chromium.org/1760823002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/splitter_test.html:46: assertEquals(100, afterWidth-beforeWidth); On 2016/03/07 18:14:19, Dan Beam wrote: > afterWidth - beforeWidth > ^ ^ Done. https://codereview.chromium.org/1760823002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/splitter_test.html:48: function testSplitter_ResizeNextElement() { On 2016/03/07 18:14:20, Dan Beam wrote: > \n Done. https://codereview.chromium.org/1760823002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/splitter_test.html:73: assertEquals(100, afterWidth-beforeWidth); On 2016/03/07 18:14:20, Dan Beam wrote: > afterWidth - beforeWidth > ^ ^ Done. https://codereview.chromium.org/1760823002/diff/20001/ui/webui/resources/js/c... File ui/webui/resources/js/cr/ui/splitter.js (right): https://codereview.chromium.org/1760823002/diff/20001/ui/webui/resources/js/c... ui/webui/resources/js/cr/ui/splitter.js:78: * @param {boolean=} resizeNext True if resize the next element. On 2016/03/07 18:14:20, Dan Beam wrote: > = means optional. this should not be optional, so I'd remove the = Done. https://codereview.chromium.org/1760823002/diff/20001/ui/webui/resources/js/c... ui/webui/resources/js/cr/ui/splitter.js:82: this.resizeNextElement_ = !!resizeNext; On 2016/03/07 18:14:20, Dan Beam wrote: > you should only accept booleans (not undefined), so !! should not be needed Done. https://codereview.chromium.org/1760823002/diff/20001/ui/webui/resources/js/c... ui/webui/resources/js/cr/ui/splitter.js:150: * Calculate witdh to resize target element. On 2016/03/07 18:14:20, Dan Beam wrote: > width Done. https://codereview.chromium.org/1760823002/diff/20001/ui/webui/resources/js/c... ui/webui/resources/js/cr/ui/splitter.js:157: +deltaX; On 2016/03/07 18:14:20, Dan Beam wrote: > return this.resizeNextElement_ ? -deltaX : deltaX; Done.
lgtm https://codereview.chromium.org/1760823002/diff/40001/ui/webui/resources/js/c... File ui/webui/resources/js/cr/ui/splitter.js (right): https://codereview.chromium.org/1760823002/diff/40001/ui/webui/resources/js/c... ui/webui/resources/js/cr/ui/splitter.js:156: return this.resizeNextElement_ ? -deltaX : +deltaX; nit: unary + doesn't do anything in this case (as |deltaX| is already a number), i'd drop it unless you really really think it adds to readability
Thank you! https://codereview.chromium.org/1760823002/diff/40001/ui/webui/resources/js/c... File ui/webui/resources/js/cr/ui/splitter.js (right): https://codereview.chromium.org/1760823002/diff/40001/ui/webui/resources/js/c... ui/webui/resources/js/cr/ui/splitter.js:156: return this.resizeNextElement_ ? -deltaX : +deltaX; On 2016/03/08 01:29:17, Dan Beam wrote: > nit: unary + doesn't do anything in this case (as |deltaX| is already a number), > i'd drop it unless you really really think it adds to readability I added '+' to make it look symmetrical, but it looks redundant now... I will remove it.
The CQ bit was checked by ryoh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/1760823002/#ps60001 (title: "remote redundant '+'")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1760823002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1760823002/60001
Message was sent while issue was closed.
Description was changed from ========== Allow splitter to resize next element To specify which element should be resized, I added a new argument "resizeNextElement" to decorate method. BUG=591611 TEST=none ========== to ========== Allow splitter to resize next element To specify which element should be resized, I added a new argument "resizeNextElement" to decorate method. BUG=591611 TEST=none ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Allow splitter to resize next element To specify which element should be resized, I added a new argument "resizeNextElement" to decorate method. BUG=591611 TEST=none ========== to ========== Allow splitter to resize next element To specify which element should be resized, I added a new argument "resizeNextElement" to decorate method. BUG=591611 TEST=none Committed: https://crrev.com/9613e62a3e9d55ffdc04d1276ee57c54ae8322e2 Cr-Commit-Position: refs/heads/master@{#379737} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/9613e62a3e9d55ffdc04d1276ee57c54ae8322e2 Cr-Commit-Position: refs/heads/master@{#379737} |
