Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1897383002:
DevTools: Update styling for rendering drawer panel (Closed)
|
Created:
4 years, 8 months ago by luoe Modified:
4 years, 7 months ago CC:
apavlov+blink_chromium.org, blink-reviews, caseq+blink_chromium.org, chromium-reviews, devtools-reviews_chromium.org, kozyatinskiy+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, sergeyv+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUpdate styling for rendering drawer panel
BUG=604833
Committed: https://crrev.com/e4172bed62d9a3ffdcecdd2ff866dc7c1090f2d1
Cr-Commit-Position: refs/heads/master@{#389296}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Address comments in patch 1 #
Total comments: 10
Patch Set 3 : Address comments in patch 2 #Patch Set 4 : Rename commit #
Total comments: 2
Messages
Total messages: 21 (9 generated)
luoe@chromium.org changed reviewers: + dgozman@chromium.org, lushnikov@chromium.org, paulirish@chromium.org, pfeldman@chromium.org
This CL mainly adds descriptions to the Rendering panel, according to Max's design
luoe@chromium.org changed reviewers: - dgozman@chromium.org, paulirish@chromium.org, pfeldman@chromium.org
Description was changed from ========== Update styling for rendering drawer panel See design slides https://docs.google.com/presentation/d/1mmGYr6pqwEC74qudxUkdvKfpC3T4FupNIfz-G... BUG=604833 ========== to ========== Update styling for rendering drawer panel BUG=604833 ==========
https://codereview.chromium.org/1897383002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/main/renderingOptions.css (right): https://codereview.chromium.org/1897383002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/main/renderingOptions.css:7: .rendering-view { can't we use :host here? https://codereview.chromium.org/1897383002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js (right): https://codereview.chromium.org/1897383002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js:1270: element.subtitleElement = element.textElement.createChild("div", "dt-checkbox-subtitle"); Don't you also need to put the default subtitle style into checkboxTextLabel.css? https://codereview.chromium.org/1897383002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/ui/inspectorCommon.css (right): https://codereview.chromium.org/1897383002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/inspectorCommon.css:276: .panel-section-separator { lets move this into renderingOptions.css In your other patch, you can simply copy this single rule
Comments addressed, ready for review.
https://codereview.chromium.org/1897383002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/main/RenderingOptions.js (left): https://codereview.chromium.org/1897383002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/main/RenderingOptions.js:47: var scrollingTitle = WebInspector.UIString("Shows areas of the page that slow down scrolling:\nTouch and mousewheel event listeners can delay scrolling.\nSome areas need to repaint their content when scrolled."); why do we drop this? Lets' still show this on hover - it's informative. https://codereview.chromium.org/1897383002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/main/renderingOptions.css (right): https://codereview.chromium.org/1897383002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/main/renderingOptions.css:17: display: inline-block; i believe this rule could be stripped down: .media-row { margin-left: 25px; } https://codereview.chromium.org/1897383002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/main/renderingOptions.css:23: .media-row > label { this is not needed https://codereview.chromium.org/1897383002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/checkboxTextLabel.css (right): https://codereview.chromium.org/1897383002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/checkboxTextLabel.css:58: ::content .dt-checkbox-text { this rule affects a lot of checkboxes in the front-end: e.g. settings screen. I doubt you meant this :) https://codereview.chromium.org/1897383002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/checkboxTextLabel.css:64: font-size: 12px; i'd strip this down to color: gray and margin-top properties; all the rest should be styled by clients.
https://codereview.chromium.org/1897383002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/main/renderingOptions.css (right): https://codereview.chromium.org/1897383002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/main/renderingOptions.css:7: .rendering-view { On 2016/04/19 22:37:45, lushnikov wrote: > can't we use :host here? Done. https://codereview.chromium.org/1897383002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js (right): https://codereview.chromium.org/1897383002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js:1270: element.subtitleElement = element.textElement.createChild("div", "dt-checkbox-subtitle"); On 2016/04/19 22:37:45, lushnikov wrote: > Don't you also need to put the default subtitle style into > checkboxTextLabel.css? Done. https://codereview.chromium.org/1897383002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/ui/inspectorCommon.css (right): https://codereview.chromium.org/1897383002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/inspectorCommon.css:276: .panel-section-separator { On 2016/04/19 22:37:45, lushnikov wrote: > lets move this into renderingOptions.css > > In your other patch, you can simply copy this single rule Done. https://codereview.chromium.org/1897383002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/main/RenderingOptions.js (left): https://codereview.chromium.org/1897383002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/main/RenderingOptions.js:47: var scrollingTitle = WebInspector.UIString("Shows areas of the page that slow down scrolling:\nTouch and mousewheel event listeners can delay scrolling.\nSome areas need to repaint their content when scrolled."); On 2016/04/20 23:50:32, lushnikov wrote: > why do we drop this? Lets' still show this on hover - it's informative. Done. https://codereview.chromium.org/1897383002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/main/renderingOptions.css (right): https://codereview.chromium.org/1897383002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/main/renderingOptions.css:17: display: inline-block; On 2016/04/20 23:50:32, lushnikov wrote: > i believe this rule could be stripped down: > > .media-row { > margin-left: 25px; > } Done. https://codereview.chromium.org/1897383002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/main/renderingOptions.css:23: .media-row > label { On 2016/04/20 23:50:32, lushnikov wrote: > this is not needed Done. https://codereview.chromium.org/1897383002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/checkboxTextLabel.css (right): https://codereview.chromium.org/1897383002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/checkboxTextLabel.css:58: ::content .dt-checkbox-text { On 2016/04/20 23:50:33, lushnikov wrote: > this rule affects a lot of checkboxes in the front-end: e.g. settings screen. I > doubt you meant this :) Done. https://codereview.chromium.org/1897383002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/checkboxTextLabel.css:64: font-size: 12px; On 2016/04/20 23:50:33, lushnikov wrote: > i'd strip this down to color: gray and margin-top properties; all the rest > should be styled by clients. I cut it down to just color: gray. Margins may vary if font-size varies.
nice, thanks! lgtm
Description was changed from ========== Update styling for rendering drawer panel BUG=604833 ========== to ========== Update styling for rendering drawer panel BUG=604833 ==========
The CQ bit was checked by luoe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lushnikov@chromium.org Link to the patchset: https://codereview.chromium.org/1897383002/#ps60001 (title: "Rename commit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1897383002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1897383002/60001
Message was sent while issue was closed.
Description was changed from ========== Update styling for rendering drawer panel BUG=604833 ========== to ========== Update styling for rendering drawer panel BUG=604833 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Update styling for rendering drawer panel BUG=604833 ========== to ========== Update styling for rendering drawer panel BUG=604833 Committed: https://crrev.com/e4172bed62d9a3ffdcecdd2ff866dc7c1090f2d1 Cr-Commit-Position: refs/heads/master@{#389296} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/e4172bed62d9a3ffdcecdd2ff866dc7c1090f2d1 Cr-Commit-Position: refs/heads/master@{#389296}
Message was sent while issue was closed.
pfeldman@chromium.org changed reviewers: + pfeldman@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1897383002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/main/renderingOptions.css (right): https://codereview.chromium.org/1897383002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/main/renderingOptions.css:26: .dt-checkbox-text { All dt- custom elements should go into UIUtils. https://codereview.chromium.org/1897383002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/main/renderingOptions.css:34: font-size: 10px; We should not use the font <12px.
Message was sent while issue was closed.
On 2016/04/30 02:53:33, pfeldman wrote: > https://codereview.chromium.org/1897383002/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/devtools/front_end/main/renderingOptions.css > (right): > > https://codereview.chromium.org/1897383002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/main/renderingOptions.css:26: > .dt-checkbox-text { > All dt- custom elements should go into UIUtils. Ah, so you only override it here... Still, they are introduced for consistency, so we should not override!
Message was sent while issue was closed.
On 2016/04/30 02:55:21, pfeldman wrote: > On 2016/04/30 02:53:33, pfeldman wrote: > > > https://codereview.chromium.org/1897383002/diff/60001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/devtools/front_end/main/renderingOptions.css > > (right): > > > > > https://codereview.chromium.org/1897383002/diff/60001/third_party/WebKit/Sour... > > third_party/WebKit/Source/devtools/front_end/main/renderingOptions.css:26: > > .dt-checkbox-text { > > All dt- custom elements should go into UIUtils. > > Ah, so you only override it here... Still, they are introduced for consistency, > so we should not override! Got it. Instead of overriding .dt-checkbox-text, should we replace it with something custom ".rendering-checkbox-text" or would a narrower scope ".rendering view .dt-checkbox-text" be fine? I'll bump the font-size on the subtitle up to 12px to meet our minimum, and increase the checkbox text to be slightly bigger. Changes will come in a separate CL! |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
