Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2628773004:
MD Settings: consolidate learn-more link styles (Closed)
|
Created:
3 years, 11 months ago by scottchen Modified:
3 years, 11 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/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: consolidate learn-more link styles
BUG=649427
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2628773004
Cr-Commit-Position: refs/heads/master@{#443298}
Committed: https://chromium.googlesource.com/chromium/src/+/861d6ea98b5b7a7c23b8c12ee6354a10411d86d4
Patch Set 1 #
Total comments: 5
Patch Set 2 : re-add css rule that was accidentally removed #
Messages
Total messages: 33 (17 generated)
Description was changed from ========== MD Settings: consolidate learn-more link styles BUG=649427 ========== to ========== MD Settings: consolidate learn-more link styles BUG=649427 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
scottchen@chromium.org changed reviewers: + hcarmona@chromium.org
scottchen@chromium.org changed reviewers: + dbeam@chromium.org
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...
scottchen@chromium.org changed reviewers: + dpapad@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
lgtm
The CQ bit was checked by scottchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2628773004/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/reset_page/reset_profile_dialog.html (left): https://codereview.chromium.org/2628773004/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/reset_page/reset_profile_dialog.html:16: .footer a { Is this now relying on settings-shared to style teh <a> tag? Perhaps a before/after screenshot would be helpful.
do all of these work/look similarly? https://codereview.chromium.org/2628773004/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/reset_page/reset_profile_dialog.html (right): https://codereview.chromium.org/2628773004/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/reset_page/reset_profile_dialog.html:27: <a href="$i18nRaw{resetPageLearnMoreUrl}" target="_blank"> <a href> https://codereview.chromium.org/2628773004/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/reset_page/reset_profile_dialog.html:39: <paper-button class="action-button" on-tap="onResetTap_" .action-button https://codereview.chromium.org/2628773004/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/reset_page/reset_profile_dialog.html:47: <a is="action-link" [is='action-link']
The CQ bit was unchecked by dbeam@chromium.org
https://codereview.chromium.org/2628773004/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/reset_page/reset_profile_dialog.html (left): https://codereview.chromium.org/2628773004/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/reset_page/reset_profile_dialog.html:16: .footer a { On 2017/01/11 22:07:15, dpapad wrote: > Is this now relying on settings-shared to style teh <a> tag? Perhaps a > before/after screenshot would be helpful. Acknowledged. And yes, it's using the shared style now.
On 2017/01/11 22:07:15, dpapad wrote: > https://codereview.chromium.org/2628773004/diff/1/chrome/browser/resources/se... > File chrome/browser/resources/settings/reset_page/reset_profile_dialog.html > (left): > > https://codereview.chromium.org/2628773004/diff/1/chrome/browser/resources/se... > chrome/browser/resources/settings/reset_page/reset_profile_dialog.html:16: > .footer a { > Is this now relying on settings-shared to style teh <a> tag? Perhaps a > before/after screenshot would be helpful. http://imgur.com/a/OPgMt
On 2017/01/11 22:11:03, Dan Beam wrote: > do all of these work/look similarly? > > https://codereview.chromium.org/2628773004/diff/1/chrome/browser/resources/se... > File chrome/browser/resources/settings/reset_page/reset_profile_dialog.html > (right): > > https://codereview.chromium.org/2628773004/diff/1/chrome/browser/resources/se... > chrome/browser/resources/settings/reset_page/reset_profile_dialog.html:27: <a > href="$i18nRaw{resetPageLearnMoreUrl}" target="_blank"> > <a href> > > https://codereview.chromium.org/2628773004/diff/1/chrome/browser/resources/se... > chrome/browser/resources/settings/reset_page/reset_profile_dialog.html:39: > <paper-button class="action-button" on-tap="onResetTap_" > .action-button > > https://codereview.chromium.org/2628773004/diff/1/chrome/browser/resources/se... > chrome/browser/resources/settings/reset_page/reset_profile_dialog.html:47: <a > is="action-link" > [is='action-link'] No, I think they're all different for good reasons in this particular page: .action-button actually looks like a button. <a href> and [is='action-link'] looks the same, but works differently - <a href> opens an external page directly, whereas [is='action-link'] calls a js function via on-tap.
On 2017/01/11 22:30:48, scottchen wrote: > On 2017/01/11 22:11:03, Dan Beam wrote: > > do all of these work/look similarly? > > > > > https://codereview.chromium.org/2628773004/diff/1/chrome/browser/resources/se... > > File chrome/browser/resources/settings/reset_page/reset_profile_dialog.html > > (right): > > > > > https://codereview.chromium.org/2628773004/diff/1/chrome/browser/resources/se... > > chrome/browser/resources/settings/reset_page/reset_profile_dialog.html:27: <a > > href="$i18nRaw{resetPageLearnMoreUrl}" target="_blank"> > > <a href> > > > > > https://codereview.chromium.org/2628773004/diff/1/chrome/browser/resources/se... > > chrome/browser/resources/settings/reset_page/reset_profile_dialog.html:39: > > <paper-button class="action-button" on-tap="onResetTap_" > > .action-button > > > > > https://codereview.chromium.org/2628773004/diff/1/chrome/browser/resources/se... > > chrome/browser/resources/settings/reset_page/reset_profile_dialog.html:47: <a > > is="action-link" > > [is='action-link'] > > No, I think they're all different for good reasons in this particular page: > .action-button actually looks like a button. > <a href> and [is='action-link'] looks the same, but works differently - <a href> > opens an external page directly, whereas [is='action-link'] calls a js function > via on-tap. right, as long as you know those are the differences. lgtm
> http://imgur.com/a/OPgMt By removing "vertical-align: middle;" the "View reported settings" text is shifted upwards (undoing the fix that was landed at https://codereview.chromium.org/1911943002/diff/1/chrome/browser/resources/se...).
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 2017/01/11 23:09:49, dpapad wrote: > > http://imgur.com/a/OPgMt > > By removing "vertical-align: middle;" the "View reported settings" text is > shifted upwards (undoing the fix that was landed at > https://codereview.chromium.org/1911943002/diff/1/chrome/browser/resources/se...). fixed: http://imgur.com/a/31FXH Please confirm, I'll wait for your lg
On 2017/01/12 at 17:37:20, scottchen wrote: > On 2017/01/11 23:09:49, dpapad wrote: > > > http://imgur.com/a/OPgMt > > > > By removing "vertical-align: middle;" the "View reported settings" text is > > shifted upwards (undoing the fix that was landed at > > https://codereview.chromium.org/1911943002/diff/1/chrome/browser/resources/se...). > > fixed: http://imgur.com/a/31FXH > > Please confirm, I'll wait for your lg LGTM, thanks.
The CQ bit was unchecked by scottchen@chromium.org
The CQ bit was checked by scottchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org, hcarmona@chromium.org Link to the patchset: https://codereview.chromium.org/2628773004/#ps20001 (title: "re-add css rule that was accidentally removed")
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": 20001, "attempt_start_ts": 1484243993492930,
"parent_rev": "b1b91cd87f42a0eff0b0037fd8e65960b6d52866", "commit_rev":
"861d6ea98b5b7a7c23b8c12ee6354a10411d86d4"}
Message was sent while issue was closed.
Description was changed from ========== MD Settings: consolidate learn-more link styles BUG=649427 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: consolidate learn-more link styles BUG=649427 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2628773004 Cr-Commit-Position: refs/heads/master@{#443298} Committed: https://chromium.googlesource.com/chromium/src/+/861d6ea98b5b7a7c23b8c12ee635... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/861d6ea98b5b7a7c23b8c12ee635... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
