Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2472083003:
Increase .settings-box and .settigns-box.two-line height. (Closed)
|
Created:
4 years, 1 month ago by scottchen Modified:
4 years, 1 month ago CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-settings_chromium.org, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, stevenjb+watch-md-settings_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIncrease .settings-box and .settigns-box.two-line height.
I created new variables for just the .settings-box classes, instead of increasing the settings-row-min-height values.
This is because .settings-row-min-height is also used by other classes (radio buttons, check boxes, list items etc), but bettes@ specifically requested that only the box container's height be increased, and leave the other classes alone.
This leads me to believe that we need separate variables for the .settings-box container so we can tweak them separately easily in the future.
BUG=652375
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/b0d714461da621694d56f8949545d7da6cb551ea
Cr-Commit-Position: refs/heads/master@{#429948}
Patch Set 1 #
Total comments: 3
Patch Set 2 : reverted the original height for .two-line and added a more specific .settings-box.two-line rule #
Total comments: 3
Patch Set 3 : added comments to differentiate different height css variables #
Total comments: 2
Patch Set 4 : wrap 80col+ comments #
Total comments: 4
Patch Set 5 : reformat comments to fit style guide #
Messages
Total messages: 39 (20 generated)
Description was changed from ========== Increase .settings-box and .settigns-box.two-line height. I created new variables for just the .settings-box classes, instead of increasing the settings-row-min-height values. This is because .settings-row-min-height is also used by other classes (radio buttons, check boxes, list items etc), but bettes@ specifically requested that only the box container's height be increased, and leave the other classes alone. This leads me to believe that we need separate variables for the .settings-box container so we can tweak them separately easily in the future. BUG=652375 ========== to ========== Increase .settings-box and .settigns-box.two-line height. I created new variables for just the .settings-box classes, instead of increasing the settings-row-min-height values. This is because .settings-row-min-height is also used by other classes (radio buttons, check boxes, list items etc), but bettes@ specifically requested that only the box container's height be increased, and leave the other classes alone. This leads me to believe that we need separate variables for the .settings-box container so we can tweak them separately easily in the future. BUG=652375 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by scottchen@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...
Description was changed from ========== Increase .settings-box and .settigns-box.two-line height. I created new variables for just the .settings-box classes, instead of increasing the settings-row-min-height values. This is because .settings-row-min-height is also used by other classes (radio buttons, check boxes, list items etc), but bettes@ specifically requested that only the box container's height be increased, and leave the other classes alone. This leads me to believe that we need separate variables for the .settings-box container so we can tweak them separately easily in the future. BUG=652375 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Increase .settings-box and .settigns-box.two-line height. I created new variables for just the .settings-box classes, instead of increasing the settings-row-min-height values. This is because .settings-row-min-height is also used by other classes (radio buttons, check boxes, list items etc), but bettes@ specifically requested that only the box container's height be increased, and leave the other classes alone. This leads me to believe that we need separate variables for the .settings-box container so we can tweak them separately easily in the future. before: https://screenshot.googleplex.com/on2ueovtCwO after: https://screenshot.googleplex.com/zZOwbmZ8yUz BUG=652375 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
scottchen@chromium.org changed reviewers: + dpapad@chromium.org
what constitutes a difference between a box and a row? https://codereview.chromium.org/2472083003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/settings_shared_css.html (right): https://codereview.chromium.org/2472083003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_shared_css.html:302: .two-line { should this be .settings-box.two-line?
The CQ bit was checked by scottchen@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...
On 2016/11/03 18:55:40, Dan Beam wrote: > what constitutes a difference between a box and a row? > > https://codereview.chromium.org/2472083003/diff/1/chrome/browser/resources/se... > File chrome/browser/resources/settings/settings_shared_css.html (right): > > https://codereview.chromium.org/2472083003/diff/1/chrome/browser/resources/se... > chrome/browser/resources/settings/settings_shared_css.html:302: .two-line { > should this be .settings-box.two-line? From what I understand, `.settings-box` is the visual boundary (i.e. with a white background and a border bottom). `.settings-box` may contain one or many separate "row-like" items, which each has the `min-height: --settings-ROW-min-height` rule on them. See: https://screenshot.googleplex.com/Zni1CT3CM6K In the screenshot, in the "on start-up" block for example, bettes@ wants the radio buttons within that one .settings-box to remain 44px. Since they each have the --settings-row-min-height applied, i'm considering each of them as their own "row" and should continue to use the --settings-row-min-height to impose the 44px min-height style.
https://codereview.chromium.org/2472083003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/settings_shared_css.html (right): https://codereview.chromium.org/2472083003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_shared_css.html:302: .two-line { On 2016/11/03 18:55:40, Dan Beam wrote: > should this be .settings-box.two-line? You're right, I missed the fact that there are other things using the .two-line class. I'll revert this .two-line rule to use the previous setting, and then add a separate .settings-box.two-line rule.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Increase .settings-box and .settigns-box.two-line height. I created new variables for just the .settings-box classes, instead of increasing the settings-row-min-height values. This is because .settings-row-min-height is also used by other classes (radio buttons, check boxes, list items etc), but bettes@ specifically requested that only the box container's height be increased, and leave the other classes alone. This leads me to believe that we need separate variables for the .settings-box container so we can tweak them separately easily in the future. before: https://screenshot.googleplex.com/on2ueovtCwO after: https://screenshot.googleplex.com/zZOwbmZ8yUz BUG=652375 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Increase .settings-box and .settigns-box.two-line height. I created new variables for just the .settings-box classes, instead of increasing the settings-row-min-height values. This is because .settings-row-min-height is also used by other classes (radio buttons, check boxes, list items etc), but bettes@ specifically requested that only the box container's height be increased, and leave the other classes alone. This leads me to believe that we need separate variables for the .settings-box container so we can tweak them separately easily in the future. BUG=652375 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
https://codereview.chromium.org/2472083003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/settings_shared_css.html (right): https://codereview.chromium.org/2472083003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_shared_css.html:302: .two-line { On 2016/11/03 19:26:22, scottchen wrote: > On 2016/11/03 18:55:40, Dan Beam wrote: > > should this be .settings-box.two-line? > > You're right, I missed the fact that there are other things using the .two-line > class. > > I'll revert this .two-line rule to use the previous setting, and then add a > separate .settings-box.two-line rule. Done.
LGTM https://codereview.chromium.org/2472083003/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/settings_vars_css.html (right): https://codereview.chromium.org/2472083003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/settings_vars_css.html:35: Perhaps worth adding a comment that those are used for rows that hold radio buttons, checkboxes or .list-items.
https://codereview.chromium.org/2472083003/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/settings_vars_css.html (right): https://codereview.chromium.org/2472083003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/settings_vars_css.html:35: On 2016/11/03 22:33:28, dpapad wrote: > Perhaps worth adding a comment that those are used for rows that hold radio > buttons, checkboxes or .list-items. Acknowledged.
https://codereview.chromium.org/2472083003/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/settings_vars_css.html (right): https://codereview.chromium.org/2472083003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/settings_vars_css.html:35: On 2016/11/03 22:39:28, scottchen wrote: > On 2016/11/03 22:33:28, dpapad wrote: > > Perhaps worth adding a comment that those are used for rows that hold radio > > buttons, checkboxes or .list-items. > > Acknowledged. Done.
The CQ bit was checked by scottchen@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/2472083003/#ps40001 (title: "added comments to differentiate different height css variables")
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 dbeam@chromium.org
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
https://codereview.chromium.org/2472083003/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/settings_vars_css.html (right): https://codereview.chromium.org/2472083003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/settings_vars_css.html:36: /* These are used for row items such as radio buttons, check boxes, list items etc */ this should wrap at 80 cols
https://codereview.chromium.org/2472083003/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/settings_vars_css.html (right): https://codereview.chromium.org/2472083003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/settings_vars_css.html:36: /* These are used for row items such as radio buttons, check boxes, list items etc */ On 2016/11/03 23:06:57, Dan Beam wrote: > this should wrap at 80 cols Done. Nice catch.
https://codereview.chromium.org/2472083003/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/settings_vars_css.html (right): https://codereview.chromium.org/2472083003/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/settings_vars_css.html:37: * check boxes, list items etc */ this should have a . at the end https://codereview.chromium.org/2472083003/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/settings_vars_css.html:37: * check boxes, list items etc */ can you expand this until a word exceeds 80 cols, then wrap? right now you're wrapping at a comma, and while that looks fairly nice, we generally just expand to as far as possible.
https://codereview.chromium.org/2472083003/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/settings_vars_css.html (right): https://codereview.chromium.org/2472083003/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/settings_vars_css.html:37: * check boxes, list items etc */ On 2016/11/04 00:30:59, Dan Beam wrote: > this should have a . at the end Done. https://codereview.chromium.org/2472083003/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/settings_vars_css.html:37: * check boxes, list items etc */ On 2016/11/04 00:30:59, Dan Beam wrote: > can you expand this until a word exceeds 80 cols, then wrap? right now you're > wrapping at a comma, and while that looks fairly nice, we generally just expand > to as far as possible. Done.
cool, thanks! lgtm
The CQ bit was checked by scottchen@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/2472083003/#ps80001 (title: "reformat comments to fit style guide")
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
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by dbeam@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Increase .settings-box and .settigns-box.two-line height. I created new variables for just the .settings-box classes, instead of increasing the settings-row-min-height values. This is because .settings-row-min-height is also used by other classes (radio buttons, check boxes, list items etc), but bettes@ specifically requested that only the box container's height be increased, and leave the other classes alone. This leads me to believe that we need separate variables for the .settings-box container so we can tweak them separately easily in the future. BUG=652375 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Increase .settings-box and .settigns-box.two-line height. I created new variables for just the .settings-box classes, instead of increasing the settings-row-min-height values. This is because .settings-row-min-height is also used by other classes (radio buttons, check boxes, list items etc), but bettes@ specifically requested that only the box container's height be increased, and leave the other classes alone. This leads me to believe that we need separate variables for the .settings-box container so we can tweak them separately easily in the future. BUG=652375 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Increase .settings-box and .settigns-box.two-line height. I created new variables for just the .settings-box classes, instead of increasing the settings-row-min-height values. This is because .settings-row-min-height is also used by other classes (radio buttons, check boxes, list items etc), but bettes@ specifically requested that only the box container's height be increased, and leave the other classes alone. This leads me to believe that we need separate variables for the .settings-box container so we can tweak them separately easily in the future. BUG=652375 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Increase .settings-box and .settigns-box.two-line height. I created new variables for just the .settings-box classes, instead of increasing the settings-row-min-height values. This is because .settings-row-min-height is also used by other classes (radio buttons, check boxes, list items etc), but bettes@ specifically requested that only the box container's height be increased, and leave the other classes alone. This leads me to believe that we need separate variables for the .settings-box container so we can tweak them separately easily in the future. BUG=652375 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/b0d714461da621694d56f8949545d7da6cb551ea Cr-Commit-Position: refs/heads/master@{#429948} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/b0d714461da621694d56f8949545d7da6cb551ea Cr-Commit-Position: refs/heads/master@{#429948} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
