Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(210)

Issue 2390053002: [MD settings] use arrow-forward to switch direction in rtl (Closed)

Created:
4 years, 2 months ago by dschuyler
Modified:
4 years, 2 months ago
Reviewers:
Dan Beam
CC:
dpapad, 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.

Description

[MD settings] use arrow-forward to switch direction in rtl This CL flips the arrow-forward to left or right based on the rtl/ltr direction setting and corrects the placement of the paper ripple under the icon on rtl. Minor changes: changes the old style name from .arrow-right to .arrow-forward (since left or right changes in rtl/ltr); there are also some css variables added to replace some constants. BUG=651716, 586579 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/f76c6502e7f4a6d57fe57aed25a60ce3845dd82a Cr-Commit-Position: refs/heads/master@{#425837}

Patch Set 1 #

Total comments: 4

Patch Set 2 : #

Patch Set 3 : #

Total comments: 6

Patch Set 4 : in person changes #

Total comments: 12

Patch Set 5 : review changes #

Total comments: 6

Patch Set 6 : review changes #

Total comments: 1

Patch Set 7 : merge with master; unwrapping lines #

Patch Set 8 : fixed indent #

Patch Set 9 : #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+62 lines, -71 lines) Patch
M chrome/browser/resources/settings/a11y_page/a11y_page.html View 1 2 3 4 5 6 1 chunk +1 line, -2 lines 0 comments Download
M chrome/browser/resources/settings/a11y_page/manage_a11y_page.html View 1 2 3 4 5 6 1 chunk +1 line, -2 lines 0 comments Download
M chrome/browser/resources/settings/appearance_page/appearance_page.html View 1 2 3 4 5 6 1 chunk +1 line, -2 lines 0 comments Download
M chrome/browser/resources/settings/device_page/device_page.html View 1 2 3 4 5 6 1 chunk +5 lines, -10 lines 0 comments Download
M chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.html View 1 2 3 4 5 6 2 chunks +2 lines, -4 lines 0 comments Download
M chrome/browser/resources/settings/people_page/people_page.html View 1 2 3 4 5 6 3 chunks +3 lines, -5 lines 0 comments Download
M chrome/browser/resources/settings/printing_page/printing_page.html View 1 2 3 4 5 6 2 chunks +2 lines, -4 lines 0 comments Download
M chrome/browser/resources/settings/privacy_page/privacy_page.html View 1 2 3 4 5 6 3 chunks +3 lines, -6 lines 0 comments Download
M chrome/browser/resources/settings/reset_page/reset_page.html View 1 2 3 4 5 6 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/resources/settings/search_page/search_page.html View 1 2 3 4 5 6 1 chunk +1 line, -2 lines 0 comments Download
M chrome/browser/resources/settings/settings_shared_css.html View 1 2 3 4 5 6 1 chunk +11 lines, -11 lines 0 comments Download
M chrome/browser/resources/settings/settings_vars_css.html View 1 2 3 4 5 6 2 chunks +7 lines, -4 lines 0 comments Download
M chrome/browser/resources/settings/site_settings/site_list.html View 1 2 3 4 5 6 7 8 1 chunk +6 lines, -0 lines 1 comment Download
M chrome/browser/resources/settings/site_settings_page/site_settings_page.html View 1 2 3 4 5 6 7 17 chunks +17 lines, -17 lines 0 comments Download

Messages

Total messages: 53 (34 generated)
dschuyler
This CL is smaller than it appears. The real changes are in settings_shared_css.html. Most of ...
4 years, 2 months ago (2016-10-03 22:02:22 UTC) #5
Dan Beam
https://codereview.chromium.org/2390053002/diff/1/chrome/browser/resources/settings/settings_shared_css.html File chrome/browser/resources/settings/settings_shared_css.html (right): https://codereview.chromium.org/2390053002/diff/1/chrome/browser/resources/settings/settings_shared_css.html#newcode37 chrome/browser/resources/settings/settings_shared_css.html:37: :host-context([dir=rtl]) [actionable] button[is="paper-icon-button-light"].icon-arrow-forward { wrap https://codereview.chromium.org/2390053002/diff/1/chrome/browser/resources/settings/settings_shared_css.html#newcode38 chrome/browser/resources/settings/settings_shared_css.html:38: background-image: url(images/arrow_left.svg); ...
4 years, 2 months ago (2016-10-03 22:04:54 UTC) #6
dschuyler
Patch#1 and Patch#3 are both viable. Patch#1 uses an arrow_left.svg file. Patch#3 uses a workaround ...
4 years, 2 months ago (2016-10-03 22:22:41 UTC) #11
dpapad
Deferring this CL to dbeam@, as a CSS expert.
4 years, 2 months ago (2016-10-03 22:48:29 UTC) #13
dpapad
4 years, 2 months ago (2016-10-03 22:48:48 UTC) #15
Dan Beam
https://codereview.chromium.org/2390053002/diff/40001/chrome/browser/resources/settings/settings_shared_css.html File chrome/browser/resources/settings/settings_shared_css.html (right): https://codereview.chromium.org/2390053002/diff/40001/chrome/browser/resources/settings/settings_shared_css.html#newcode13 chrome/browser/resources/settings/settings_shared_css.html:13: top: -8px; why can't we just write the code ...
4 years, 2 months ago (2016-10-03 23:42:06 UTC) #18
dschuyler
I marked the review comments ack because we talked in person. The new changes are ...
4 years, 2 months ago (2016-10-04 00:56:27 UTC) #21
Dan Beam
https://codereview.chromium.org/2390053002/diff/60001/chrome/browser/resources/settings/a11y_page/a11y_page.html File chrome/browser/resources/settings/a11y_page/a11y_page.html (right): https://codereview.chromium.org/2390053002/diff/60001/chrome/browser/resources/settings/a11y_page/a11y_page.html#newcode33 chrome/browser/resources/settings/a11y_page/a11y_page.html:33: <button class="icon-arrow-forward" is="paper-icon-button-light"> while we're renaming this, why not ...
4 years, 2 months ago (2016-10-04 01:01:01 UTC) #22
dschuyler
https://codereview.chromium.org/2390053002/diff/60001/chrome/browser/resources/settings/a11y_page/a11y_page.html File chrome/browser/resources/settings/a11y_page/a11y_page.html (right): https://codereview.chromium.org/2390053002/diff/60001/chrome/browser/resources/settings/a11y_page/a11y_page.html#newcode33 chrome/browser/resources/settings/a11y_page/a11y_page.html:33: <button class="icon-arrow-forward" is="paper-icon-button-light"> On 2016/10/04 01:01:00, Dan Beam wrote: ...
4 years, 2 months ago (2016-10-04 01:13:58 UTC) #25
Dan Beam
https://codereview.chromium.org/2390053002/diff/80001/chrome/browser/resources/settings/a11y_page/a11y_page.html File chrome/browser/resources/settings/a11y_page/a11y_page.html (right): https://codereview.chromium.org/2390053002/diff/80001/chrome/browser/resources/settings/a11y_page/a11y_page.html#newcode33 chrome/browser/resources/settings/a11y_page/a11y_page.html:33: <button class="icon-subpage-arrow" is="paper-icon-button-light"> i meant class="subpage-arrow" (i.e. drop the ...
4 years, 2 months ago (2016-10-04 02:08:40 UTC) #26
dschuyler
https://codereview.chromium.org/2390053002/diff/80001/chrome/browser/resources/settings/a11y_page/a11y_page.html File chrome/browser/resources/settings/a11y_page/a11y_page.html (right): https://codereview.chromium.org/2390053002/diff/80001/chrome/browser/resources/settings/a11y_page/a11y_page.html#newcode33 chrome/browser/resources/settings/a11y_page/a11y_page.html:33: <button class="icon-subpage-arrow" is="paper-icon-button-light"> On 2016/10/04 02:08:39, Dan Beam wrote: ...
4 years, 2 months ago (2016-10-04 18:38:45 UTC) #31
Dan Beam
lgtm https://codereview.chromium.org/2390053002/diff/100001/chrome/browser/resources/settings/default_browser_page/default_browser_page.html File chrome/browser/resources/settings/default_browser_page/default_browser_page.html (right): https://codereview.chromium.org/2390053002/diff/100001/chrome/browser/resources/settings/default_browser_page/default_browser_page.html#newcode24 chrome/browser/resources/settings/default_browser_page/default_browser_page.html:24: </button> you get to unwrap more things! lucky ...
4 years, 2 months ago (2016-10-14 02:27:45 UTC) #35
dschuyler
unwrapped lines
4 years, 2 months ago (2016-10-17 21:05:10 UTC) #40
Dan Beam
lgtm
4 years, 2 months ago (2016-10-17 21:11:17 UTC) #41
dschuyler
A small change to blend with the enableSiteSettings flag. https://codereview.chromium.org/2390053002/diff/160001/chrome/browser/resources/settings/site_settings/site_list.html File chrome/browser/resources/settings/site_settings/site_list.html (right): https://codereview.chromium.org/2390053002/diff/160001/chrome/browser/resources/settings/site_settings/site_list.html#newcode91 chrome/browser/resources/settings/site_settings/site_list.html:91: ...
4 years, 2 months ago (2016-10-17 21:16:36 UTC) #44
Dan Beam
lgtm
4 years, 2 months ago (2016-10-17 21:17:44 UTC) #45
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2390053002/160001
4 years, 2 months ago (2016-10-18 00:39:32 UTC) #49
commit-bot: I haz the power
Committed patchset #9 (id:160001)
4 years, 2 months ago (2016-10-18 00:44:34 UTC) #51
commit-bot: I haz the power
4 years, 2 months ago (2016-10-18 00:48:31 UTC) #53
Message was sent while issue was closed.
Patchset 9 (id:??) landed as
https://crrev.com/f76c6502e7f4a6d57fe57aed25a60ce3845dd82a
Cr-Commit-Position: refs/heads/master@{#425837}

Powered by Google App Engine
This is Rietveld 408576698