Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2628633002:
WebUI: cr-slider: Fix layout with large fonts (Closed)
|
Created:
3 years, 11 months ago by stevenjb Modified:
3 years, 11 months ago Reviewers:
michaelpg CC:
chromium-reviews, michaelpg+watch-elements_chromium.org, stevenjb+watch-md-settings_chromium.org, dbeam+watch-elements_chromium.org, oshima+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWebUI: cr-slider: Fix layout with large fonts
paper-slider uses a fixed size of 200px.
This does not work well with cr-slider when the size of the labels
increases. Instead, set the minimum width of cr-slider to be 200px,
and the width of the embedded paper-slider to 100%.
BUG=679319
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2628633002
Cr-Commit-Position: refs/heads/master@{#443091}
Committed: https://chromium.googlesource.com/chromium/src/+/2c9cbe19a0fcbf5386caec36010d201ec3430713
Patch Set 1 #
Total comments: 4
Patch Set 2 : Feedback #Messages
Total messages: 15 (7 generated)
Description was changed from ========== WebUI: cr-slider: Fix layout with large fonts BUG=679319 ========== to ========== WebUI: cr-slider: Fix layout with large fonts BUG=679319 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== WebUI: cr-slider: Fix layout with large fonts BUG=679319 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== WebUI: cr-slider: Fix layout with large fonts paper-slider uses a fixed size of 200px. This does not work well with cr-slider when the size of the labels increases. Instead, set the minimum width of cr-slider to be 200px, and the width of the embedded paper-slider to 100%. BUG=679319 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== WebUI: cr-slider: Fix layout with large fonts paper-slider uses a fixed size of 200px. This does not work well with cr-slider when the size of the labels increases. Instead, set the minimum width of cr-slider to be 200px, and the width of the embedded paper-slider to 100%. BUG=679319 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== WebUI: cr-slider: Fix layout with large fonts paper-slider uses a fixed size of 200px. This does not work well with cr-slider when the size of the labels increases. Instead, set the minimum width of cr-slider to be 200px, and the width of the embedded paper-slider to 100%. BUG=679319 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
stevenjb@chromium.org changed reviewers: + michaelpg@chromium.org
https://codereview.chromium.org/2628633002/diff/1/ui/webui/resources/cr_eleme... File ui/webui/resources/cr_elements/cr_slider/cr_slider.html (right): https://codereview.chromium.org/2628633002/diff/1/ui/webui/resources/cr_eleme... ui/webui/resources/cr_elements/cr_slider/cr_slider.html:16: div#labels { do you need this "div" at the beginning? what does it do for you? https://codereview.chromium.org/2628633002/diff/1/ui/webui/resources/cr_eleme... ui/webui/resources/cr_elements/cr_slider/cr_slider.html:24: font-size: 12px; why is this using a px-based font-size?
https://codereview.chromium.org/2628633002/diff/1/ui/webui/resources/cr_eleme... File ui/webui/resources/cr_elements/cr_slider/cr_slider.html (right): https://codereview.chromium.org/2628633002/diff/1/ui/webui/resources/cr_eleme... ui/webui/resources/cr_elements/cr_slider/cr_slider.html:16: div#labels { On 2017/01/10 20:12:47, Dan Beam wrote: > do you need this "div" at the beginning? what does it do for you? Done. https://codereview.chromium.org/2628633002/diff/1/ui/webui/resources/cr_eleme... ui/webui/resources/cr_elements/cr_slider/cr_slider.html:24: font-size: 12px; On 2017/01/10 20:12:47, Dan Beam wrote: > why is this using a px-based font-size? It's not clear to me whether this should change with the default font size, since the slider itself does not. I assumed that it shouldn't. I asked Alan/Tom in the issue for this bug. (FWIW, Paper element labels do not scale).
Does this mean increasing the width of the slider? is that what UX wants for other controls?
On 2017/01/10 22:31:00, michaelpg (NYC) wrote: > Does this mean increasing the width of the slider? is that what UX wants for > other controls? It may increase the width of the slider for extremely large fonts. The alternative is pretty ugly... cr-slider is kind of special, I'm not sure that other controls will have this problem.
lgtm On 2017/01/10 22:33:02, stevenjb wrote: > On 2017/01/10 22:31:00, michaelpg (NYC) wrote: > > Does this mean increasing the width of the slider? is that what UX wants for > > other controls? > > It may increase the width of the slider for extremely large fonts. > The alternative is pretty ugly... yeah, i guess it would be > cr-slider is kind of special, I'm not sure that other controls will have this > problem.
The CQ bit was checked by stevenjb@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": 1484176086541020,
"parent_rev": "145b27b6e14ae6dd557c823e39680d62f16b1cf3", "commit_rev":
"2c9cbe19a0fcbf5386caec36010d201ec3430713"}
Message was sent while issue was closed.
Description was changed from ========== WebUI: cr-slider: Fix layout with large fonts paper-slider uses a fixed size of 200px. This does not work well with cr-slider when the size of the labels increases. Instead, set the minimum width of cr-slider to be 200px, and the width of the embedded paper-slider to 100%. BUG=679319 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== WebUI: cr-slider: Fix layout with large fonts paper-slider uses a fixed size of 200px. This does not work well with cr-slider when the size of the labels increases. Instead, set the minimum width of cr-slider to be 200px, and the width of the embedded paper-slider to 100%. BUG=679319 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2628633002 Cr-Commit-Position: refs/heads/master@{#443091} Committed: https://chromium.googlesource.com/chromium/src/+/2c9cbe19a0fcbf5386caec36010d... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/2c9cbe19a0fcbf5386caec36010d... |
