Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2843333002:
WebUI: restrict cr-dialogs max-height to the viewport. (Closed)
|
Created:
3 years, 8 months ago by scottchen Modified:
3 years, 7 months ago Reviewers:
dpapad CC:
chromium-reviews, dbeam+watch-elements_chromium.org, michaelpg+watch-elements_chromium.org, oshima+watch_chromium.org, stevenjb+watch-md-settings_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionWebUI: restrict cr-dialogs max-height to the viewport.
BUG=713663, 713548
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2843333002
Cr-Commit-Position: refs/heads/master@{#468722}
Committed: https://chromium.googlesource.com/chromium/src/+/fc2f3e78a68e400ab08ab5f0396fb77428ea4860
Patch Set 1 #
Total comments: 2
Patch Set 2 : change to use CSS only #Patch Set 3 : add a minimally reasonable height to body-container #Patch Set 4 : change display to flex only for open dialog #Patch Set 5 : fix test #
Messages
Total messages: 30 (20 generated)
Description was changed from ========== WebUI: Dynamically shrink cr-dialogs height when it doesn't fit the viewport. BUG=713663 ========== to ========== WebUI: Dynamically shrink cr-dialogs height when it doesn't fit the viewport. BUG=713663 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
scottchen@chromium.org changed reviewers: + dpapad@chromium.org
https://codereview.chromium.org/2843333002/diff/1/ui/webui/resources/cr_eleme... File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js (right): https://codereview.chromium.org/2843333002/diff/1/ui/webui/resources/cr_eleme... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js:103: adjustHeight_: function() { Can we add a test for the auto-shrink logic? Also, does this look jarring to the user (things jump around)?
Description was changed from ========== WebUI: Dynamically shrink cr-dialogs height when it doesn't fit the viewport. BUG=713663 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== WebUI: restrict cr-dialogs max-height to the viewport. BUG=713663 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
after/before screenshot: http://imgur.com/a/QhtGZ https://codereview.chromium.org/2843333002/diff/1/ui/webui/resources/cr_eleme... File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js (right): https://codereview.chromium.org/2843333002/diff/1/ui/webui/resources/cr_eleme... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js:103: adjustHeight_: function() { On 2017/04/27 22:33:24, dpapad wrote: > Can we add a test for the auto-shrink logic? Also, does this look jarring to the > user (things jump around)? It did not jump around. However, based on our offline conversation I've switched to a more manageable CSS-only solution.
LGTM. I am glad we landed on a CSS only solution for this. Just one question: What happens when you make the viewport height very very small? Should we add a reasonable min-height to the dialog to account for it?
On 2017/04/28 23:57:53, dpapad wrote: > LGTM. I am glad we landed on a CSS only solution for this. Just one question: > What happens when you make the viewport height very very small? Should we add a > reasonable min-height to the dialog to account for it? Added, PTAL.
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/04/29 at 00:20:48, scottchen wrote: > On 2017/04/28 23:57:53, dpapad wrote: > > LGTM. I am glad we landed on a CSS only solution for this. Just one question: > > What happens when you make the viewport height very very small? Should we add a > > reasonable min-height to the dialog to account for it? > > Added, PTAL. Sill 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...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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 dpapad@chromium.org Link to the patchset: https://codereview.chromium.org/2843333002/#ps80001 (title: "fix test")
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": 80001, "attempt_start_ts": 1493749303363760,
"parent_rev": "c631069dc9ea490001bec2329aeb9dc2af8b51dd", "commit_rev":
"fc2f3e78a68e400ab08ab5f0396fb77428ea4860"}
Message was sent while issue was closed.
Description was changed from ========== WebUI: restrict cr-dialogs max-height to the viewport. BUG=713663 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== WebUI: restrict cr-dialogs max-height to the viewport. BUG=713663 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2843333002 Cr-Commit-Position: refs/heads/master@{#468722} Committed: https://chromium.googlesource.com/chromium/src/+/fc2f3e78a68e400ab08ab5f0396f... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/fc2f3e78a68e400ab08ab5f0396f...
Message was sent while issue was closed.
Description was changed from ========== WebUI: restrict cr-dialogs max-height to the viewport. BUG=713663 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2843333002 Cr-Commit-Position: refs/heads/master@{#468722} Committed: https://chromium.googlesource.com/chromium/src/+/fc2f3e78a68e400ab08ab5f0396f... ========== to ========== WebUI: restrict cr-dialogs max-height to the viewport. BUG=713663, 713548 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2843333002 Cr-Commit-Position: refs/heads/master@{#468722} Committed: https://chromium.googlesource.com/chromium/src/+/fc2f3e78a68e400ab08ab5f0396f... ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
