Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2623053002:
[Devtools] Fix left margin for selection element (Closed)
|
Created:
3 years, 11 months ago by aboxhall Modified:
3 years, 11 months ago Reviewers:
dgozman CC:
chromium-reviews, caseq+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, pfeldman, kozyatinskiy+blink_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Devtools] Fix left margin for selection element
BUG=679918
Review-Url: https://codereview.chromium.org/2623053002
Cr-Commit-Position: refs/heads/master@{#444990}
Committed: https://chromium.googlesource.com/chromium/src/+/5104b622941be693bc4d9dadb12425d21bd5bc76
Patch Set 1 #Patch Set 2 : Remove leading underscore #
Total comments: 4
Patch Set 3 : dgozman comments #
Total comments: 4
Patch Set 4 : more comments #
Total comments: 4
Patch Set 5 : dgozman comments #
Messages
Total messages: 18 (7 generated)
Description was changed from ========== [Devtools] Fix left margin for selection element BUG=679918 ========== to ========== [Devtools] Fix left margin for selection element BUG=679918 ==========
aboxhall@chromium.org changed reviewers: + dgozman@chromium.org
Bump?
Patchset #3 (id:40001) has been deleted
Any chance you could take a look at this?
Sorry for late review, was too busy. https://codereview.chromium.org/2623053002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeElement.js (right): https://codereview.chromium.org/2623053002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeElement.js:1020: computeLeftIndent() { I'm worried that this clashes with treeoutline's one, since we do a lot of custom things here. Let's keep them separate. https://codereview.chromium.org/2623053002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js (right): https://codereview.chromium.org/2623053002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:324: this.paddingSize = 12; Default should be zero preserving -10000px we have now, or else this will regress some trees.
https://codereview.chromium.org/2623053002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeElement.js (right): https://codereview.chromium.org/2623053002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeElement.js:1020: computeLeftIndent() { On 2017/01/13 01:53:42, dgozman wrote: > I'm worried that this clashes with treeoutline's one, since we do a lot of > custom things here. Let's keep them separate. Done. https://codereview.chromium.org/2623053002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js (right): https://codereview.chromium.org/2623053002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:324: this.paddingSize = 12; On 2017/01/13 01:53:42, dgozman wrote: > Default should be zero preserving -10000px we have now, or else this will > regress some trees. Done.
https://codereview.chromium.org/2623053002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js (right): https://codereview.chromium.org/2623053002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:767: treeElement = treeElement.parent; Shouldn't we sum up all paddingSize for parent elements instead? But then it's hard to disable the feature (as I want it to be by default to not break anything). Let's instead introduce TreeOutline.setPaddingSize(padding) and use a single value in all tree elements. https://codereview.chromium.org/2623053002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:778: this._selectionElement.style.setProperty('margin-left', this.computeLeftMargin() + 'px'); Don't even set this value when it's zero by default.
https://codereview.chromium.org/2623053002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js (right): https://codereview.chromium.org/2623053002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:767: treeElement = treeElement.parent; On 2017/01/18 16:06:08, dgozman wrote: > Shouldn't we sum up all paddingSize for parent elements instead? But then it's > hard to disable the feature (as I want it to be by default to not break > anything). > Let's instead introduce TreeOutline.setPaddingSize(padding) and use a single > value in all tree elements. Done. https://codereview.chromium.org/2623053002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:778: this._selectionElement.style.setProperty('margin-left', this.computeLeftMargin() + 'px'); On 2017/01/18 16:06:08, dgozman wrote: > Don't even set this value when it's zero by default. Done.
lgtm https://codereview.chromium.org/2623053002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/accessibility/AXTreePane.js (right): https://codereview.chromium.org/2623053002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/accessibility/AXTreePane.js:183: this.paddingSize = 12; Remove. https://codereview.chromium.org/2623053002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js (right): https://codereview.chromium.org/2623053002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:215: setPaddingSize(paddingSize) { JSDoc please.
The CQ bit was checked by aboxhall@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org Link to the patchset: https://codereview.chromium.org/2623053002/#ps100001 (title: "dgozman comments")
https://codereview.chromium.org/2623053002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/accessibility/AXTreePane.js (right): https://codereview.chromium.org/2623053002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/accessibility/AXTreePane.js:183: this.paddingSize = 12; On 2017/01/19 23:12:45, dgozman wrote: > Remove. Done. https://codereview.chromium.org/2623053002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js (right): https://codereview.chromium.org/2623053002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:215: setPaddingSize(paddingSize) { On 2017/01/19 23:12:45, dgozman wrote: > JSDoc please. Done.
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": 100001, "attempt_start_ts": 1484886292578560,
"parent_rev": "6795958b17b2aacf0ff1053d3df0948a3e4a1ad4", "commit_rev":
"5104b622941be693bc4d9dadb12425d21bd5bc76"}
Message was sent while issue was closed.
Description was changed from ========== [Devtools] Fix left margin for selection element BUG=679918 ========== to ========== [Devtools] Fix left margin for selection element BUG=679918 Review-Url: https://codereview.chromium.org/2623053002 Cr-Commit-Position: refs/heads/master@{#444990} Committed: https://chromium.googlesource.com/chromium/src/+/5104b622941be693bc4d9dadb124... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001) as https://chromium.googlesource.com/chromium/src/+/5104b622941be693bc4d9dadb124... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
