Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2795763003:
MD Settings: adjust dialog style and scroll border visual (Closed)
|
Created:
3 years, 8 months ago by scottchen Modified:
3 years, 8 months 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/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: adjust dialog style and scroll border visual
BUG=707631
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2795763003
Cr-Commit-Position: refs/heads/master@{#463373}
Committed: https://chromium.googlesource.com/chromium/src/+/fee40485e1568f0a7121576d722f2b66a5832c65
Patch Set 1 #Patch Set 2 : clean up intersectionObserver, add tests #
Total comments: 10
Patch Set 3 : revert body-padding shrink since it's adding scroll-bars to body #Patch Set 4 : modify test and updates based on comments #Patch Set 5 : add missing var declaration #
Total comments: 6
Patch Set 6 : updates based on comments #Patch Set 7 : explicitly assert intersection entries max length #
Total comments: 4
Patch Set 8 : consolidate styles #
Total comments: 3
Patch Set 9 : audited and cleaned up rogue dialog stylings. #Patch Set 10 : modify cups dialog button to align by flexbox #Messages
Total messages: 49 (24 generated)
Description was changed from ========== MD Settings: remove double-bordering in scrollable dialog BUG=707631 ========== to ========== MD Settings: remove double-bordering in scrollable dialog BUG=707631 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
scottchen@chromium.org changed reviewers: + dschuyler@chromium.org
screenshot after fix: http://imgur.com/a/MaswS (before shot included in bug report)
On 2017/04/03 17:47:32, scottchen wrote: > screenshot after fix: http://imgur.com/a/MaswS (before shot included in bug > report) It looks like comment #3 just got added to the bug :) Also, what happens when scrolled to top, is there still a line between the title the body?
On 2017/04/03 23:09:39, dschuyler wrote: > On 2017/04/03 17:47:32, scottchen wrote: > > screenshot after fix: http://imgur.com/a/MaswS (before shot included in bug > > report) > > It looks like comment #3 just got added to the bug :) > Also, what happens when scrolled to top, is there still a line between the title > the body? Currently in the CL - yes, since the bottom-border is on the dialog header. After new spec - not sure. Will hold off until bettes@ gets back to the bug.
On 2017/04/03 23:24:35, scottchen wrote: > On 2017/04/03 23:09:39, dschuyler wrote: > > On 2017/04/03 17:47:32, scottchen wrote: > > > screenshot after fix: http://imgur.com/a/MaswS (before shot included in bug > > > report) > > > > It looks like comment #3 just got added to the bug :) > > Also, what happens when scrolled to top, is there still a line between the > title > > the body? > > > Currently in the CL - yes, since the bottom-border is on the dialog header. > After new spec - not sure. > > Will hold off until bettes@ gets back to the bug. ok, thanks.
Description was changed from ========== MD Settings: remove double-bordering in scrollable dialog BUG=707631 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: adjust dialog style and scroll border visual BUG=707631 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Drive by comments. https://codereview.chromium.org/2795763003/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/cr_elements/cr_dialog_test.js (right): https://codereview.chromium.org/2795763003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/cr_elements/cr_dialog_test.js:131: 'class') // Only care about class mutations Formatting seems a bit off here. Can we move the comment to a separate line and keep the condition to a single line? https://codereview.chromium.org/2795763003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/cr_elements/cr_dialog_test.js:136: console.log(bodyContainer.classList[0]); Remove console.log. https://codereview.chromium.org/2795763003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/cr_elements/cr_dialog_test.js:140: } else if (observerCount == 2) { // Triggered when scrolled back to top. Is there a way to test the case where both top and bottom borders are showing? https://codereview.chromium.org/2795763003/diff/20001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html (right): https://codereview.chromium.org/2795763003/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html:44: font-size: 115.38%; /* (15px / 13px) * 100 */ Can we use rem units to avoid those complex calculations?
https://codereview.chromium.org/2795763003/diff/20001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html (right): https://codereview.chromium.org/2795763003/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html:44: font-size: 115.38%; /* (15px / 13px) * 100 */ On 2017/04/05 23:56:11, dpapad wrote: > Can we use rem units to avoid those complex calculations? I'm not seeing where this would be better font-size: 1.1538em; /* (15px / 13px) */
https://codereview.chromium.org/2795763003/diff/20001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html (right): https://codereview.chromium.org/2795763003/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html:44: font-size: 115.38%; /* (15px / 13px) * 100 */ On 2017/04/06 00:53:27, dschuyler wrote: > On 2017/04/05 23:56:11, dpapad wrote: > > Can we use rem units to avoid those complex calculations? > > I'm not seeing where this would be better > font-size: 1.1538em; /* (15px / 13px) */ Though we should probably chat about it outside of this CL. The % version is the standard in MD settings at the moment. If we change it to em we should change it consistently (which would be outside of the scope of this CL).
dpapad@chromium.org changed reviewers: + dpapad@chromium.org
https://codereview.chromium.org/2795763003/diff/20001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html (right): https://codereview.chromium.org/2795763003/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html:44: font-size: 115.38%; /* (15px / 13px) * 100 */ On 2017/04/06 at 00:54:57, dschuyler wrote: > On 2017/04/06 00:53:27, dschuyler wrote: > > On 2017/04/05 23:56:11, dpapad wrote: > > > Can we use rem units to avoid those complex calculations? > > > > I'm not seeing where this would be better > > font-size: 1.1538em; /* (15px / 13px) */ > > Though we should probably chat about it outside of this CL. The % version is the standard in MD settings at the moment. If we change it to em we should change it consistently (which would be outside of the scope of this CL). rem units are different than em units. em: Relative to the font-size of the element (2em means 2 times the size of the current font) rem: Relative to font-size of the root element The latter makes it much easier to derive all fonts based on a single value without caring about the DOM hierarchy. We have already been using rem units in a few places, see https://cs.chromium.org/search/?q=%22rem;%22+file:%5Esrc/chrome/browser/resou.... I agree that this can be discussed outside the context of this CL though.
The CQ bit was checked by scottchen@chromium.org to run a CQ dry run
https://codereview.chromium.org/2795763003/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/cr_elements/cr_dialog_test.js (right): https://codereview.chromium.org/2795763003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/cr_elements/cr_dialog_test.js:131: 'class') // Only care about class mutations On 2017/04/05 23:56:10, dpapad wrote: > Formatting seems a bit off here. Can we move the comment to a separate line and > keep the condition to a single line? Done. https://codereview.chromium.org/2795763003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/cr_elements/cr_dialog_test.js:136: console.log(bodyContainer.classList[0]); On 2017/04/05 23:56:10, dpapad wrote: > Remove console.log. Done. https://codereview.chromium.org/2795763003/diff/20001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html (right): https://codereview.chromium.org/2795763003/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html:44: font-size: 115.38%; /* (15px / 13px) * 100 */ On 2017/04/06 00:54:57, dschuyler wrote: > On 2017/04/06 00:53:27, dschuyler wrote: > > On 2017/04/05 23:56:11, dpapad wrote: > > > Can we use rem units to avoid those complex calculations? > > > > I'm not seeing where this would be better > > font-size: 1.1538em; /* (15px / 13px) */ > > Though we should probably chat about it outside of this CL. The % version is the > standard in MD settings at the moment. If we change it to em we should change it > consistently (which would be outside of the scope of this CL). Like Dave said, rem wouldn't be any shorter in this case. I think we should just overhaul how we set font-sizes and line-heights in MD settings after beta launch.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dpapad@chromium.org changed reviewers: - dpapad@chromium.org
FYI (I also left this comment in the bug): Due to weirdness in how <paper-input> display error-messages, if we shrink the dialog body padding, then it will start to show unnecessary scroll-bars on many dialog body at medium font (it's currently already a problem for large font). So for now I'll be leaving body padding at 12px.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2795763003/diff/80001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html (right): https://codereview.chromium.org/2795763003/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html:44: font-size: 115.38%; /* (15px / 13px) * 100 */ Two spaces between ; and comment on inline comments. font-size: 115.38%; /* (15px / 13px) * 100 */ https://codereview.chromium.org/2795763003/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html:95: -webkit-padding-start: 16px; How about padding: 0 16px; (There's nothing special needed for -end and -start if they are the same). https://codereview.chromium.org/2795763003/diff/80001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js (right): https://codereview.chromium.org/2795763003/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js:72: var callback = function(entries) { How large can |entries.length| be?
https://codereview.chromium.org/2795763003/diff/80001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html (right): https://codereview.chromium.org/2795763003/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html:44: font-size: 115.38%; /* (15px / 13px) * 100 */ On 2017/04/06 21:53:22, dschuyler wrote: > Two spaces between ; and comment on inline comments. > font-size: 115.38%; /* (15px / 13px) * 100 */ Done. https://codereview.chromium.org/2795763003/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html:95: -webkit-padding-start: 16px; On 2017/04/06 21:53:22, dschuyler wrote: > How about > padding: 0 16px; > (There's nothing special needed for -end and -start if they are the same). Done. https://codereview.chromium.org/2795763003/diff/80001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js (right): https://codereview.chromium.org/2795763003/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js:72: var callback = function(entries) { On 2017/04/06 21:53:22, dschuyler wrote: > How large can |entries.length| be? At most 2, added an assert to call it out.
lgtm https://codereview.chromium.org/2795763003/diff/120001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html (right): https://codereview.chromium.org/2795763003/diff/120001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html:47: margin-top: 16px; nit: margin: 16px 0; can replace lines 46 and 47. (And the style guide says "Use shorthand notation when possible") https://codereview.chromium.org/2795763003/diff/120001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html:103: margin-top: 16px; margin: 16px 0;
https://codereview.chromium.org/2795763003/diff/120001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html (right): https://codereview.chromium.org/2795763003/diff/120001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html:47: margin-top: 16px; On 2017/04/06 22:29:13, dschuyler wrote: > nit: > margin: 16px 0; > can replace lines 46 and 47. > (And the style guide says "Use shorthand notation when possible") Done. https://codereview.chromium.org/2795763003/diff/120001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html:103: margin-top: 16px; On 2017/04/06 22:29:13, dschuyler wrote: > margin: 16px 0; Done.
The CQ bit was checked by scottchen@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/2795763003/#ps140001 (title: "consolidate styles")
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
scottchen@chromium.org changed reviewers: + dpapad@chromium.org
need LG from owner
LGTM with question. https://codereview.chromium.org/2795763003/diff/140001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html (right): https://codereview.chromium.org/2795763003/diff/140001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html:83: border-top: 1px solid var(--paper-grey-300); Can we declare a variable in :host{} and reuse it here and a few lines above? :host { ... --border: 1px solid var(--paper-grey-300); } :host([show-scroll-borders]) .body-container.top-scrollable { border-top: var(--border); } https://codereview.chromium.org/2795763003/diff/140001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html:97: padding: 0 16px; Are you changing this just to compact things? Or does the top/bottom padding needs to be zero? The previous code was such that it did not specify padding top/bottom, on purpose I think, but not sure. Either way, I see one case where a client is specifying a padding-top at https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/printi.... Can you verify that this is not breaking the layout of that dialog?
Patchset #11 (id:200001) has been deleted
Patchset #10 (id:180001) has been deleted
Patchset #9 (id:160001) has been deleted
Alright, sorry for sneaking in more stuff after TWO LGs! Following reviewers' suggestion to audit usages of cr_dialogs, I found a few outdated styles in a few places, so I brushed them up to spec. One worth calling out is the cups-add-printer dialogs. I changed the styling around its button. However, I left the body height and button-container border untouched. (Talked to xdai@ on chromeOS UI, they exist for good reasons) before/after shots of the add-printer dialog: http://imgur.com/a/Jt1DY https://codereview.chromium.org/2795763003/diff/140001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html (right): https://codereview.chromium.org/2795763003/diff/140001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html:83: border-top: 1px solid var(--paper-grey-300); On 2017/04/06 23:50:55, dpapad wrote: > Can we declare a variable in :host{} and reuse it here and a few lines above? > > :host { > ... > --border: 1px solid var(--paper-grey-300); > } > > :host([show-scroll-borders]) .body-container.top-scrollable { > border-top: var(--border); > } Done.
On 2017/04/07 at 22:55:54, scottchen wrote: > Alright, sorry for sneaking in more stuff after TWO LGs! > > Following reviewers' suggestion to audit usages of cr_dialogs, I found a few outdated styles in a few places, so I brushed them up to spec. > > One worth calling out is the cups-add-printer dialogs. I changed the styling around its button. However, I left the body height and button-container border untouched. (Talked to xdai@ on chromeOS UI, they exist for good reasons) > > before/after shots of the add-printer dialog: http://imgur.com/a/Jt1DY > The "add manually" button left alignment seems a bit off in the "after" I think it used to be aligned with the elements above it, now it is not. Can you take a look?
On 2017/04/07 23:40:10, dpapad wrote: > On 2017/04/07 at 22:55:54, scottchen wrote: > > Alright, sorry for sneaking in more stuff after TWO LGs! > > > > Following reviewers' suggestion to audit usages of cr_dialogs, I found a few > outdated styles in a few places, so I brushed them up to spec. > > > > One worth calling out is the cups-add-printer dialogs. I changed the styling > around its button. However, I left the body height and button-container border > untouched. (Talked to xdai@ on chromeOS UI, they exist for good reasons) > > > > before/after shots of the add-printer dialog: http://imgur.com/a/Jt1DY > > > > The "add manually" button left alignment seems a bit off in the "after" I think > it used to be aligned with the elements above it, now it is not. Can you take a > look? Found out it was originally using margin-left on the left button to hard-code spacing in between, which is fragile and breaks when font-size changes. I've modified it to use flexbox and grouped the left- and right-aligning buttons in separate divs. PTAL at patch #10 (and screenshot here: http://imgur.com/a/4MLjp)
On 2017/04/07 at 23:50:08, scottchen wrote: > On 2017/04/07 23:40:10, dpapad wrote: > > On 2017/04/07 at 22:55:54, scottchen wrote: > > > Alright, sorry for sneaking in more stuff after TWO LGs! > > > > > > Following reviewers' suggestion to audit usages of cr_dialogs, I found a few > > outdated styles in a few places, so I brushed them up to spec. > > > > > > One worth calling out is the cups-add-printer dialogs. I changed the styling > > around its button. However, I left the body height and button-container border > > untouched. (Talked to xdai@ on chromeOS UI, they exist for good reasons) > > > > > > before/after shots of the add-printer dialog: http://imgur.com/a/Jt1DY > > > > > > > The "add manually" button left alignment seems a bit off in the "after" I think > > it used to be aligned with the elements above it, now it is not. Can you take a > > look? > > Found out it was originally using margin-left on the left button to hard-code spacing in between, which is fragile and breaks when font-size changes. > > I've modified it to use flexbox and grouped the left- and right-aligning buttons in separate divs. PTAL at patch #10 (and screenshot here: http://imgur.com/a/4MLjp) Still LGTM.
The CQ bit was checked by scottchen@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/2795763003/#ps240001 (title: "modify cups dialog button to align by flexbox")
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": 240001, "attempt_start_ts": 1491844095410120,
"parent_rev": "1e0366fc5f961223c1862ff96a0bcd1302df32cd", "commit_rev":
"fee40485e1568f0a7121576d722f2b66a5832c65"}
Message was sent while issue was closed.
Description was changed from ========== MD Settings: adjust dialog style and scroll border visual BUG=707631 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: adjust dialog style and scroll border visual BUG=707631 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2795763003 Cr-Commit-Position: refs/heads/master@{#463373} Committed: https://chromium.googlesource.com/chromium/src/+/fee40485e1568f0a7121576d722f... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:240001) as https://chromium.googlesource.com/chromium/src/+/fee40485e1568f0a7121576d722f... |
