Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1937183002:
DevTools: fix styles in rendering pane (Closed)
|
Created:
4 years, 7 months ago by luoe Modified:
4 years, 7 months ago Reviewers:
pfeldman 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, sergeyv+blink_chromium.org, kozyatinskiy+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDevTools: fix styles in rendering pane
Addresses comments posted after the following CL was closed https://codereview.chromium.org/1897383002/
BUG=604833
Committed: https://crrev.com/ef2f7ebc4a9c7d84813da0770248f8db612d0e3c
Cr-Commit-Position: refs/heads/master@{#391351}
Patch Set 1 #Patch Set 2 : Made style more generic #
Total comments: 3
Patch Set 3 : Address comments #Patch Set 4 : Remove unnecessary style rule #Patch Set 5 : Normalize font sizes #Patch Set 6 : Rebase #
Messages
Total messages: 28 (12 generated)
Description was changed from ========== DevTools: fix styles in rendering pane Addresses comments posted after the following CL was closed https://codereview.chromium.org/1897383002/ BUG=604833 ========== to ========== DevTools: fix styles in rendering pane Addresses comments posted after the following CL was closed https://codereview.chromium.org/1897383002/ BUG=604833 ==========
luoe@chromium.org changed reviewers: + pfeldman@chromium.org
https://codereview.chromium.org/1937183002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js (right): https://codereview.chromium.org/1937183002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js:1285: element.textElement.classList.add("small-subtitle"); Do you want to add this style to the subtitle, not the text? Also, you already have a selector for it above.
https://codereview.chromium.org/1937183002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js (right): https://codereview.chromium.org/1937183002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js:1285: element.textElement.classList.add("small-subtitle"); On 2016/05/02 17:31:52, pfeldman wrote: > Do you want to add this style to the subtitle, not the text? Also, you already > have a selector for it above. Really, I want style for the text to change, bigger font, added margin. Maybe "with-subtitle" or "has-subtitle" is a better name?
https://codereview.chromium.org/1937183002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js (right): https://codereview.chromium.org/1937183002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js:1285: element.textElement.classList.add("small-subtitle"); > Really, I want style for the text to change, bigger font, added margin. Maybe > "with-subtitle" or "has-subtitle" is a better name? I don't think the text style should change based on the subtitle existence.
On 2016/05/02 18:51:52, pfeldman wrote: > https://codereview.chromium.org/1937183002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js (right): > > https://codereview.chromium.org/1937183002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js:1285: > element.textElement.classList.add("small-subtitle"); > > Really, I want style for the text to change, bigger font, added margin. Maybe > > "with-subtitle" or "has-subtitle" is a better name? > > I don't think the text style should change based on the subtitle existence. If we kept the text style the same, we would have checkboxes consistent with what we had before. According to the new design spec: http://imgur.com/EzQSAcM The new checkboxes have a lot more subtle changes, that require the style of dt-checkbox-text to change, unless we create a new component just for this drawer. Are there other cases of components with one-off styles? I think these changes belong in dt-checkbox, but could live in a new component as well.
On 2016/05/02 21:37:38, luoe wrote: > On 2016/05/02 18:51:52, pfeldman wrote: > > > https://codereview.chromium.org/1937183002/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js (right): > > > > > https://codereview.chromium.org/1937183002/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js:1285: > > element.textElement.classList.add("small-subtitle"); > > > Really, I want style for the text to change, bigger font, added margin. > Maybe > > > "with-subtitle" or "has-subtitle" is a better name? > > > > I don't think the text style should change based on the subtitle existence. > > If we kept the text style the same, we would have checkboxes consistent with > what we had before. > > According to the new design spec: http://imgur.com/EzQSAcM > The new checkboxes have a lot more subtle changes, that require the style of > dt-checkbox-text to change, unless we create a new component just for this > drawer. I'd say we ignore it since it suggests using 10px.
Alright, the latest patch should have all checkbox-related styles in checkboxTextLabel.css, reusable everywhere! Font sizes are 12px for both text and subtitles. Screenshot: http://imgur.com/ONtQnd9
lgtm
The CQ bit was checked by luoe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1937183002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1937183002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
The CQ bit was checked by luoe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1937183002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1937183002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...) win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by luoe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1937183002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1937183002/100001
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 luoe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pfeldman@chromium.org Link to the patchset: https://codereview.chromium.org/1937183002/#ps100001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1937183002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1937183002/100001
Message was sent while issue was closed.
Description was changed from ========== DevTools: fix styles in rendering pane Addresses comments posted after the following CL was closed https://codereview.chromium.org/1897383002/ BUG=604833 ========== to ========== DevTools: fix styles in rendering pane Addresses comments posted after the following CL was closed https://codereview.chromium.org/1897383002/ BUG=604833 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== DevTools: fix styles in rendering pane Addresses comments posted after the following CL was closed https://codereview.chromium.org/1897383002/ BUG=604833 ========== to ========== DevTools: fix styles in rendering pane Addresses comments posted after the following CL was closed https://codereview.chromium.org/1897383002/ BUG=604833 Committed: https://crrev.com/ef2f7ebc4a9c7d84813da0770248f8db612d0e3c Cr-Commit-Position: refs/heads/master@{#391351} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/ef2f7ebc4a9c7d84813da0770248f8db612d0e3c Cr-Commit-Position: refs/heads/master@{#391351} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
