Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2403833002:
Slightly increase the size of the CBD dialog body to hint at scrolling (Closed)
|
Created:
4 years, 2 months ago by msramek Modified:
4 years, 2 months ago CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, arv+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSlightly increase the size of the CBD dialog body to hint at scrolling
When opened in a small browser window, the CBD dialog is set to a size
in which 4 checkboxes are visible*, and the remaining 4 are below the
fold and need to be scrolled to.
[*]the label of the 5th is slightly visible if checked
While the scrollbar is usually visible, this is not the case on Mac
(where it's only visible while scrolling is in progress). Other visual
cues for scrolling are not applicable - the top and bottom border of
paper-dialog-scrollable are not visible because the dialog body has its
own borders, and shadows are not yet implemented.
As discussed in the linked bug, until a general solution is applied to
all such MD scrolling dialogs, we need a one-off solution for this case
as it's particularly sensitive. This CL increases the size of the dialog
body to hold 4.5 checkboxes, so the need to scroll is obvious. Note that
this was also the original design; this CL fixes a regression.
BUG=652027
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/ae25baeb7a5af9dad6ae2f8ad050d5564301d30d
Cr-Commit-Position: refs/heads/master@{#424400}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Added a comment. #
Total comments: 1
Messages
Total messages: 23 (13 generated)
Description was changed from ========== Slightly increase the size of the CBD dialog body to hint at scrolling BUG=652027 ========== to ========== Slightly increase the size of the CBD dialog body to hint at scrolling BUG=652027 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Slightly increase the size of the CBD dialog body to hint at scrolling BUG=652027 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Slightly increase the size of the CBD dialog body to hint at scrolling When opened in a small browser window, the CBD dialog is set to a size in which 4 checkboxes are visible*, and the remaining 4 are below the fold and need to be scrolled to. [*]the label of the 5th is slightly visible if checked While the scrollbar is usually visible, this is not the case on Mac (where it's only visible while scrolling is in progress). Other visual cues for scrolling are not applicable - the top and bottom border of paper-dialog-scrollable are not visible because the dialog body has its own borders, and shadows are not yet implemented. As discussed in the linked bug, until a general solution is applied to all such MD scrolling dialogs, we need a one-off solution for this case as it's particularly sensitive. This CL increases the size of the dialog body to hold 4.5 checkboxes, so the need to scroll is obvious. Note that this was also the original design; this CL fixes a regression. BUG=652027 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
msramek@chromium.org changed reviewers: + dschuyler@chromium.org
Hi Dave, Please have a look! I attached screenshots to the bug. Thanks, Martin
After a comment is added LGTM. https://codereview.chromium.org/2403833002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html (right): https://codereview.chromium.org/2403833002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html:105: max-height: 280px; Please add a comment or reference to the CL or bug so that this is less likely to be accidentally regressed :) Maybe: /* Intentionally show four and a *half* items in the list. See Chromium CL 2403833002 for more information. */ (Feel free to comment something else or use that directly).
The CQ bit was checked by msramek@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...
Thanks! https://codereview.chromium.org/2403833002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html (right): https://codereview.chromium.org/2403833002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html:105: max-height: 280px; On 2016/10/10 18:23:04, dschuyler wrote: > Please add a comment or reference to the CL or bug > so that this is less likely to be accidentally > regressed :) Maybe: > /* Intentionally show four and a *half* items in the list. See Chromium > CL 2403833002 for more information. */ > (Feel free to comment something else or use that directly). Done. I linked to the bug rather than the CL since there's more discussion there.
The CQ bit was unchecked by msramek@chromium.org
The CQ bit was checked by msramek@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dschuyler@chromium.org Link to the patchset: https://codereview.chromium.org/2403833002/#ps20001 (title: "Added a comment.")
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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by msramek@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 ========== Slightly increase the size of the CBD dialog body to hint at scrolling When opened in a small browser window, the CBD dialog is set to a size in which 4 checkboxes are visible*, and the remaining 4 are below the fold and need to be scrolled to. [*]the label of the 5th is slightly visible if checked While the scrollbar is usually visible, this is not the case on Mac (where it's only visible while scrolling is in progress). Other visual cues for scrolling are not applicable - the top and bottom border of paper-dialog-scrollable are not visible because the dialog body has its own borders, and shadows are not yet implemented. As discussed in the linked bug, until a general solution is applied to all such MD scrolling dialogs, we need a one-off solution for this case as it's particularly sensitive. This CL increases the size of the dialog body to hold 4.5 checkboxes, so the need to scroll is obvious. Note that this was also the original design; this CL fixes a regression. BUG=652027 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Slightly increase the size of the CBD dialog body to hint at scrolling When opened in a small browser window, the CBD dialog is set to a size in which 4 checkboxes are visible*, and the remaining 4 are below the fold and need to be scrolled to. [*]the label of the 5th is slightly visible if checked While the scrollbar is usually visible, this is not the case on Mac (where it's only visible while scrolling is in progress). Other visual cues for scrolling are not applicable - the top and bottom border of paper-dialog-scrollable are not visible because the dialog body has its own borders, and shadows are not yet implemented. As discussed in the linked bug, until a general solution is applied to all such MD scrolling dialogs, we need a one-off solution for this case as it's particularly sensitive. This CL increases the size of the dialog body to hold 4.5 checkboxes, so the need to scroll is obvious. Note that this was also the original design; this CL fixes a regression. BUG=652027 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Slightly increase the size of the CBD dialog body to hint at scrolling When opened in a small browser window, the CBD dialog is set to a size in which 4 checkboxes are visible*, and the remaining 4 are below the fold and need to be scrolled to. [*]the label of the 5th is slightly visible if checked While the scrollbar is usually visible, this is not the case on Mac (where it's only visible while scrolling is in progress). Other visual cues for scrolling are not applicable - the top and bottom border of paper-dialog-scrollable are not visible because the dialog body has its own borders, and shadows are not yet implemented. As discussed in the linked bug, until a general solution is applied to all such MD scrolling dialogs, we need a one-off solution for this case as it's particularly sensitive. This CL increases the size of the dialog body to hold 4.5 checkboxes, so the need to scroll is obvious. Note that this was also the original design; this CL fixes a regression. BUG=652027 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Slightly increase the size of the CBD dialog body to hint at scrolling When opened in a small browser window, the CBD dialog is set to a size in which 4 checkboxes are visible*, and the remaining 4 are below the fold and need to be scrolled to. [*]the label of the 5th is slightly visible if checked While the scrollbar is usually visible, this is not the case on Mac (where it's only visible while scrolling is in progress). Other visual cues for scrolling are not applicable - the top and bottom border of paper-dialog-scrollable are not visible because the dialog body has its own borders, and shadows are not yet implemented. As discussed in the linked bug, until a general solution is applied to all such MD scrolling dialogs, we need a one-off solution for this case as it's particularly sensitive. This CL increases the size of the dialog body to hold 4.5 checkboxes, so the need to scroll is obvious. Note that this was also the original design; this CL fixes a regression. BUG=652027 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/ae25baeb7a5af9dad6ae2f8ad050d5564301d30d Cr-Commit-Position: refs/heads/master@{#424400} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/ae25baeb7a5af9dad6ae2f8ad050d5564301d30d Cr-Commit-Position: refs/heads/master@{#424400}
Message was sent while issue was closed.
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2403833002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html (right): https://codereview.chromium.org/2403833002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html:106: max-height: 280px; could this be expressed as calc(4.5 * var(--some-variable))? also, does this work for all cases (i.e. if I uncheck some of these <settings-checkbox>es, they shorten, right?)
Message was sent while issue was closed.
https://codereview.chromium.org/2403833002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html (right): https://codereview.chromium.org/2403833002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html:105: max-height: 280px; On 2016/10/10 18:23:04, dschuyler wrote: > Please add a comment or reference to the CL or bug > so that this is less likely to be accidentally > regressed :) Maybe: > /* Intentionally show four and a *half* items in the list. See Chromium > CL 2403833002 for more information. */ > (Feel free to comment something else or use that directly). we also generally discourage use of * or _ or / for emphasis in comments |
