Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2837763002:
MD WebUI: Update action/cancel button height to 36px (Closed)
|
Created:
3 years, 8 months ago by tsergeant Modified:
3 years, 8 months ago Reviewers:
Dan Beam CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org, michaelpg+watch-elements_chromium.org, stevenjb+watch-md-settings_chromium.org, dbeam+watch-elements_chromium.org, oshima+watch_chromium.org, sammiequon Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD WebUI: Fix Action/Cancel button padding in MD History/Bookmarks
This fixes an issue where action/cancel buttons in MD History/Bookmarks
(and possibly other pages) had no vertical padding, after the default
paper-button padding was disabled in crrev.com/466545.
BUG=703998
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2837763002
Cr-Commit-Position: refs/heads/master@{#467238}
Committed: https://chromium.googlesource.com/chromium/src/+/b546eb97eb9f5c8683f0560a625208d5217a3d16
Patch Set 1 #
Total comments: 2
Patch Set 2 : Use padding-left/right in this CL #Messages
Total messages: 22 (12 generated)
Description was changed from ========== MD WebUI: Update action/cancel button height to 36px This fixes an issue where these buttons had no vertical padding in MD History/Bookmarks, and makes the sizing consistent across pages. BUG=703998 ========== to ========== MD WebUI: Update action/cancel button height to 36px This fixes an issue where these buttons had no vertical padding in MD History/Bookmarks, and makes the sizing consistent across pages. BUG=703998 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
tsergeant@chromium.org changed reviewers: + dbeam@chromium.org
+dbeam@ for review, cc sammiequon@ https://codereview.chromium.org/2831963004 causes problems with button sizing in MD Bookmarks/History, so this fixes it while making a consistency change that we needed eventually anyway. It looks from the bug like the original CL might need to be merged, so if it's easier to make this change somewhere else, go ahead.
The CQ bit was checked by tsergeant@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/2837763002/diff/1/ui/webui/resources/cr_eleme... File ui/webui/resources/cr_elements/shared_style_css.html (right): https://codereview.chromium.org/2837763002/diff/1/ui/webui/resources/cr_eleme... ui/webui/resources/cr_elements/shared_style_css.html:38: height: 36px; what happens with different-sized fonts?
https://codereview.chromium.org/2837763002/diff/1/ui/webui/resources/cr_eleme... File ui/webui/resources/cr_elements/shared_style_css.html (right): https://codereview.chromium.org/2837763002/diff/1/ui/webui/resources/cr_eleme... ui/webui/resources/cr_elements/shared_style_css.html:38: height: 36px; On 2017/04/24 19:08:42, Dan Beam wrote: > what happens with different-sized fonts? This is the same style that MD Settings uses everywhere (https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/settin...), so it will behave the same as that (https://i.imgur.com/0j26nbn.png)
Description was changed from ========== MD WebUI: Update action/cancel button height to 36px This fixes an issue where these buttons had no vertical padding in MD History/Bookmarks, and makes the sizing consistent across pages. BUG=703998 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD WebUI: Fix Action/Cancel button padding in MD History/Bookmarks This fixes an issue where action/cancel buttons in MD History/Bookmarks (and possibly other pages) had no vertical padding, after the default paper-button padding was disabled in crrev.com/466545. BUG=703998 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
As discussed on chat, I've simplified this CL to only fix the immediate issue. We can follow-up on the consistent sizing later.
lgtm but does this mean you're relying on the default padding-top/bottom of paper-button? do we want to specify our own px default instead?
Well, the default padding is what we'd previously been using, and it ends up fairly close to the desired value (33px vs 36px). I'm happy to keep it like this until we can unify it across pages.
The CQ bit was checked by tsergeant@chromium.org
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_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by tsergeant@chromium.org
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": 1493179695218540,
"parent_rev": "79e34734afd5d8e49b76b63beab52618344184c2", "commit_rev":
"b546eb97eb9f5c8683f0560a625208d5217a3d16"}
Message was sent while issue was closed.
Description was changed from ========== MD WebUI: Fix Action/Cancel button padding in MD History/Bookmarks This fixes an issue where action/cancel buttons in MD History/Bookmarks (and possibly other pages) had no vertical padding, after the default paper-button padding was disabled in crrev.com/466545. BUG=703998 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD WebUI: Fix Action/Cancel button padding in MD History/Bookmarks This fixes an issue where action/cancel buttons in MD History/Bookmarks (and possibly other pages) had no vertical padding, after the default paper-button padding was disabled in crrev.com/466545. BUG=703998 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2837763002 Cr-Commit-Position: refs/heads/master@{#467238} Committed: https://chromium.googlesource.com/chromium/src/+/b546eb97eb9f5c8683f0560a6252... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/b546eb97eb9f5c8683f0560a6252... |
