Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2762823002:
MD Settings: minor cleanup of content settings code. (Closed)
|
Created:
3 years, 9 months ago by scottchen Modified:
3 years, 9 months ago Reviewers:
dschuyler 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, dpapad Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: minor cleanup of content settings code.
BUG=699292
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2762823002
Cr-Commit-Position: refs/heads/master@{#459305}
Committed: https://chromium.googlesource.com/chromium/src/+/11e79c01af8a36643e8f28200107ac139726745e
Patch Set 1 #
Total comments: 6
Patch Set 2 : remove primary-toggle from content settings #Patch Set 3 : remove more primary-toggles.. #Patch Set 4 : format #
Total comments: 4
Patch Set 5 : fixes based on comments #Patch Set 6 : fixes based on comments #
Messages
Total messages: 38 (24 generated)
Description was changed from ========== MD Settings: unify subpage toggles and content styles. BUG=699292 ========== to ========== MD Settings: unify subpage toggles and content styles. BUG=699292 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
scottchen@chromium.org changed reviewers: + dschuyler@chromium.org
https://codereview.chromium.org/2762823002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/privacy_page/privacy_page.html (right): https://codereview.chromium.org/2762823002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/privacy_page/privacy_page.html:449: <settings-toggle-button class="start primary-toggle" Sorry, where is primary-toggle defined? https://codereview.chromium.org/2762823002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/site_settings/category_default_setting.html (right): https://codereview.chromium.org/2762823002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/site_settings/category_default_setting.html:25: </template> Can the goal here be achieve with hidden? A template has some weight (cost) to it. Often the weight of the template is offset by the contents it is containing (i.e. by ratio the template not that much). On a div (which is optimized/prioritized by Chrome) the template seems like a lot to hide something tiny.
On 2017/03/22 20:30:57, dschuyler wrote: > https://codereview.chromium.org/2762823002/diff/1/chrome/browser/resources/se... > File chrome/browser/resources/settings/privacy_page/privacy_page.html (right): > > https://codereview.chromium.org/2762823002/diff/1/chrome/browser/resources/se... > chrome/browser/resources/settings/privacy_page/privacy_page.html:449: > <settings-toggle-button class="start primary-toggle" > Sorry, where is primary-toggle defined? > > https://codereview.chromium.org/2762823002/diff/1/chrome/browser/resources/se... > File > chrome/browser/resources/settings/site_settings/category_default_setting.html > (right): > > https://codereview.chromium.org/2762823002/diff/1/chrome/browser/resources/se... > chrome/browser/resources/settings/site_settings/category_default_setting.html:25: > </template> > Can the goal here be achieve with hidden? A template has some weight (cost) to > it. Often the weight of the template is offset by the contents it is containing > (i.e. by ratio the template not that much). On a div (which is > optimized/prioritized by Chrome) the template seems like a lot to hide something > tiny. It's in the "Depends on patchset" patchset. https://codereview.chromium.org/2762433002
On 2017/03/22 20:42:43, scottchen wrote: > On 2017/03/22 20:30:57, dschuyler wrote: > > > https://codereview.chromium.org/2762823002/diff/1/chrome/browser/resources/se... > > File chrome/browser/resources/settings/privacy_page/privacy_page.html (right): > > > > > https://codereview.chromium.org/2762823002/diff/1/chrome/browser/resources/se... > > chrome/browser/resources/settings/privacy_page/privacy_page.html:449: > > <settings-toggle-button class="start primary-toggle" > > Sorry, where is primary-toggle defined? > > > > > https://codereview.chromium.org/2762823002/diff/1/chrome/browser/resources/se... > > File > > chrome/browser/resources/settings/site_settings/category_default_setting.html > > (right): > > > > > https://codereview.chromium.org/2762823002/diff/1/chrome/browser/resources/se... > > > chrome/browser/resources/settings/site_settings/category_default_setting.html:25: > > </template> > > Can the goal here be achieve with hidden? A template has some weight (cost) to > > it. Often the weight of the template is offset by the contents it is > containing > > (i.e. by ratio the template not that much). On a div (which is > > optimized/prioritized by Chrome) the template seems like a lot to hide > something > > tiny. > > It's in the "Depends on patchset" patchset. > https://codereview.chromium.org/2762433002 Offline I was told that the toggle (only the top toggle) changes between grey and blue to signal that the rest of the controls on the page are in effect (or not in effect). Can we use the secondary line in the toggle to explain what is happening rather than change text between blue and grey (which has no commonly understood meaning). The color change would also (I believe) be an a11y issue due to the low contrast of the change; the colors not having intrinsic meaning; and (unless it's addressed elsewhere) a screen reader won't automatically explain the the grey/blue change and the meaning. Please help me better understand why this change should be made or help me push for a different change (instead of just changing the text color).
On 2017/03/22 20:56:37, dschuyler wrote: > On 2017/03/22 20:42:43, scottchen wrote: > > On 2017/03/22 20:30:57, dschuyler wrote: > > > > > > https://codereview.chromium.org/2762823002/diff/1/chrome/browser/resources/se... > > > File chrome/browser/resources/settings/privacy_page/privacy_page.html > (right): > > > > > > > > > https://codereview.chromium.org/2762823002/diff/1/chrome/browser/resources/se... > > > chrome/browser/resources/settings/privacy_page/privacy_page.html:449: > > > <settings-toggle-button class="start primary-toggle" > > > Sorry, where is primary-toggle defined? > > > > > > > > > https://codereview.chromium.org/2762823002/diff/1/chrome/browser/resources/se... > > > File > > > > chrome/browser/resources/settings/site_settings/category_default_setting.html > > > (right): > > > > > > > > > https://codereview.chromium.org/2762823002/diff/1/chrome/browser/resources/se... > > > > > > chrome/browser/resources/settings/site_settings/category_default_setting.html:25: > > > </template> > > > Can the goal here be achieve with hidden? A template has some weight (cost) > to > > > it. Often the weight of the template is offset by the contents it is > > containing > > > (i.e. by ratio the template not that much). On a div (which is > > > optimized/prioritized by Chrome) the template seems like a lot to hide > > something > > > tiny. > > > > It's in the "Depends on patchset" patchset. > > https://codereview.chromium.org/2762433002 > > Offline I was told that the toggle (only the top toggle) changes between grey > and blue to signal that the rest of the controls on the page are in effect (or > not in effect). Can we use the secondary line in the toggle to explain what is > happening rather than change text between blue and grey (which has no commonly > understood meaning). The color change would also (I believe) be an a11y issue > due to the low contrast of the change; the colors not having intrinsic meaning; > and (unless it's addressed elsewhere) a screen reader won't automatically > explain the the grey/blue change and the meaning. Please help me better > understand why this change should be made or help me push for a different change > (instead of just changing the text color). We had an offline discussion with bettes@, dpapad@, hcarmona@, and concluded that we'll currently revert the change to style the toggle differently since its functionally different than the other on/off toggles in other sections. I'll lighten this CL to have just the layout fixes.
The CQ bit was checked by scottchen@chromium.org to run a CQ dry run
Description was changed from ========== MD Settings: unify subpage toggles and content styles. BUG=699292 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: minor cleanup of content settings code. BUG=699292 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dschuyler@ CL ready for review again. Same things sans primary-toggle. https://codereview.chromium.org/2762823002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/privacy_page/privacy_page.html (right): https://codereview.chromium.org/2762823002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/privacy_page/privacy_page.html:449: <settings-toggle-button class="start primary-toggle" On 2017/03/22 20:30:57, dschuyler wrote: > Sorry, where is primary-toggle defined? linked in previous reply. https://codereview.chromium.org/2762823002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/site_settings/category_default_setting.html (right): https://codereview.chromium.org/2762823002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/site_settings/category_default_setting.html:25: </template> On 2017/03/22 20:30:57, dschuyler wrote: > Can the goal here be achieve with hidden? A template has some weight (cost) to > it. Often the weight of the template is offset by the contents it is containing > (i.e. by ratio the template not that much). On a div (which is > optimized/prioritized by Chrome) the template seems like a lot to hide something > tiny. Done.
lgtm https://codereview.chromium.org/2762823002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/category_default_setting.js (right): https://codereview.chromium.org/2762823002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/category_default_setting.js:57: value: null // Needs default value so binding fires upon initialization. nit: Maybe a comma after null? https://codereview.chromium.org/2762823002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/category_default_setting.js:221: * @param {boolean} subOptionLabel @return {string}
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...)
https://codereview.chromium.org/2762823002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/category_default_setting.js (right): https://codereview.chromium.org/2762823002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/category_default_setting.js:57: value: null // Needs default value so binding fires upon initialization. On 2017/03/23 00:27:39, dschuyler wrote: > nit: Maybe a comma after null? Done. https://codereview.chromium.org/2762823002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/category_default_setting.js:221: * @param {boolean} subOptionLabel On 2017/03/23 00:27:39, dschuyler wrote: > @return {string} 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/2762823002/#ps80001 (title: "fixes based on comments")
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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
dpapad@chromium.org changed reviewers: + dpapad@chromium.org
Drive-by nit https://codereview.chromium.org/2762823002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/site_settings/category_default_setting.html (right): https://codereview.chromium.org/2762823002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/site_settings/category_default_setting.html:20: <div class$="settings-box {{subOptionClass_(subOptionSecondary)}}"> Do you need a two way binding here {{...}} instead of [[...]]? It seems unnecessary.
dpapad@chromium.org changed reviewers: - dpapad@chromium.org
https://codereview.chromium.org/2762823002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/site_settings/category_default_setting.html (right): https://codereview.chromium.org/2762823002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/site_settings/category_default_setting.html:20: <div class$="settings-box {{subOptionClass_(subOptionSecondary)}}"> On 2017/03/23 20:23:49, dpapad wrote: > Do you need a two way binding here {{...}} instead of [[...]]? It seems > unnecessary. Done.
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.
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/2762823002/#ps100001 (title: "fixes based on comments")
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": 100001, "attempt_start_ts": 1490315936894310,
"parent_rev": "2f3d26477b0cf74265f99dd09c9f48fa33b73850", "commit_rev":
"11e79c01af8a36643e8f28200107ac139726745e"}
Message was sent while issue was closed.
Description was changed from ========== MD Settings: minor cleanup of content settings code. BUG=699292 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: minor cleanup of content settings code. BUG=699292 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2762823002 Cr-Commit-Position: refs/heads/master@{#459305} Committed: https://chromium.googlesource.com/chromium/src/+/11e79c01af8a36643e8f28200107... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/11e79c01af8a36643e8f28200107... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
