[WebUI Refresh] update sliders |
||||||||
Issue description
,
Jun 6 2018
,
Jun 7 2018
,
Jun 15 2018
Showing focused state for slider. The spec calls for a different handling of discrete values and continuous. More information is required about this. All sliders are discrete. bettes@, namratakannan@: The following assumptions have been made in the implementation. Please take a look and correct where needed. In settings for ticks 10 or under, we show the markers evenly spaced across the slider line. I enabled the pin in this situation as well. Instead of only showing the markers to the left of the knob (dir=LTR), all the markers are shown on the line. It's simpler than calculating which markers are to the right of the knob. If necessary, we can hide them. For ticks over 10, we're treating it like the continuous case.
,
Jun 15 2018
Screenshots were from CL https://chromium-review.googlesource.com/c/chromium/src/+/1102158
,
Jun 19 2018
I talked with namratakanna@ offline for guidance on the questions I had in comment 4. Markers that span the entire slider is desirable. The condition for when to show the pin and markers should not change. It's up to the use case of whatever is using the slider. Also the updated slider style should be used in place of paper-slider wherever possible.
,
Jun 22 2018
,
Jun 22 2018
The few places that have markers and snap-to-value are in CrOS in display modes, keyboard and mouse/touch device settings. This change as I understand the spec would introduce a pin to these sliders. The underlying value for the slider is the index of the value from a predefined array of values. We can show the value in the pin, but that would require some changes to either the pin or text size for numbers with four digits. The value in the pin is also not clear since it does not show units. Do we need to show the pin at all for settings?
,
Jun 22 2018
,
Jun 22 2018
ccing Sebastien to understand if these settings need to show the values for the markers or not...
,
Jun 22 2018
Is segbsg and sgabriel the same person? I think you meant to cc sgabriel, instead.
,
Jun 25 2018
Pin or something else, we need to show the value. As you said it is used in display but more importantly in it used in Night Light where it's used to define Night light time threshold activation. See preview attached.
,
Jun 25 2018
I looked at the code behind the display settings page. The display size and night schedule slider are both custom sliders that do not share the same components as other settings sliders. Seeing as the sliders have custom implementations, the initial style refresh CL will not impact these sliders. The settings sliders for keyboard and mouse did not have a pin or label for the current value, and I opted to not add the pin in the initial CL. Should the pin/label styles of these custom sliders be changed to match the slider spec in this issue? At the least it looks like custom sliders style should be changed to match the common slider's refreshed style. Should we add a pin to the sliders under keyboard and mouse settings? What should it looks like for a value like 2000ms vs 150ms? Should it have the same unit like ms throughout the slider or should it change from (s) to (ms) at some point?
,
Jun 25 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/744ac3c75d5f60a59f810c9e262e33332c2822d5 commit 744ac3c75d5f60a59f810c9e262e33332c2822d5 Author: Esmael El-Moslimany <aee@chromium.org> Date: Mon Jun 25 21:30:29 2018 WebUI: slider style refresh cr-slider wraps paper-slider and applies the necessary style changes. paper-slider needed two small changes to allow this to work. This CL changes the style for sliders in settings. A subsequent CL will change the slider styles outside of settings (all usages of paper-slider will be replaced with cr-slider). Bug: 850243 Cq-Include-Trybots: luci.chromium.try:closure_compilation Change-Id: I70771521ff2d4cb7ec9ef69b0dca7cc0e436ad5f Reviewed-on: https://chromium-review.googlesource.com/1102158 Commit-Queue: Esmael El-Moslimany <aee@chromium.org> Reviewed-by: Scott Chen <scottchen@chromium.org> Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org> Cr-Commit-Position: refs/heads/master@{#570186} [modify] https://crrev.com/744ac3c75d5f60a59f810c9e262e33332c2822d5/chrome/browser/resources/settings/controls/BUILD.gn [modify] https://crrev.com/744ac3c75d5f60a59f810c9e262e33332c2822d5/chrome/browser/resources/settings/controls/settings_slider.html [modify] https://crrev.com/744ac3c75d5f60a59f810c9e262e33332c2822d5/chrome/browser/resources/settings/controls/settings_slider.js [modify] https://crrev.com/744ac3c75d5f60a59f810c9e262e33332c2822d5/chrome/test/data/webui/settings/device_page_tests.js [modify] https://crrev.com/744ac3c75d5f60a59f810c9e262e33332c2822d5/chrome/test/data/webui/settings/settings_slider_tests.js [modify] https://crrev.com/744ac3c75d5f60a59f810c9e262e33332c2822d5/third_party/polymer/README.chromium [modify] https://crrev.com/744ac3c75d5f60a59f810c9e262e33332c2822d5/third_party/polymer/v1_0/chromium.patch [modify] https://crrev.com/744ac3c75d5f60a59f810c9e262e33332c2822d5/third_party/polymer/v1_0/components-chromium/paper-slider/paper-slider-extracted.js [modify] https://crrev.com/744ac3c75d5f60a59f810c9e262e33332c2822d5/third_party/polymer/v1_0/components-chromium/paper-slider/paper-slider.html [modify] https://crrev.com/744ac3c75d5f60a59f810c9e262e33332c2822d5/ui/webui/resources/cr_elements/BUILD.gn [add] https://crrev.com/744ac3c75d5f60a59f810c9e262e33332c2822d5/ui/webui/resources/cr_elements/cr_slider/BUILD.gn [add] https://crrev.com/744ac3c75d5f60a59f810c9e262e33332c2822d5/ui/webui/resources/cr_elements/cr_slider/cr_slider.html [add] https://crrev.com/744ac3c75d5f60a59f810c9e262e33332c2822d5/ui/webui/resources/cr_elements/cr_slider/cr_slider.js [modify] https://crrev.com/744ac3c75d5f60a59f810c9e262e33332c2822d5/ui/webui/resources/cr_elements_resources.grdp
,
Jun 25 2018
It looks like the new pin design doesn't support well showing more than a single or 2 digit value. I'd propose to retain the current pin design for the custom sliders while updating everything else. No need for pin for sliders currently not showing any. Does that make sense?
,
Jun 29 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/946fd58e553c2fb42cbe32962e77a4741ac1af01 commit 946fd58e553c2fb42cbe32962e77a4741ac1af01 Author: Esmael El-Moslimany <aee@chromium.org> Date: Fri Jun 29 04:16:06 2018 Settings: updating custom CrOS slider styles to match cr-slider Bug: 850243 Cq-Include-Trybots: luci.chromium.try:closure_compilation Change-Id: I7f028f7541a60f0aa15495823969fdd243609845 Reviewed-on: https://chromium-review.googlesource.com/1115589 Commit-Queue: Esmael El-Moslimany <aee@chromium.org> Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org> Cr-Commit-Position: refs/heads/master@{#571383} [modify] https://crrev.com/946fd58e553c2fb42cbe32962e77a4741ac1af01/chrome/browser/resources/settings/device_page/display_size_slider.html [modify] https://crrev.com/946fd58e553c2fb42cbe32962e77a4741ac1af01/chrome/browser/resources/settings/device_page/display_size_slider.js [modify] https://crrev.com/946fd58e553c2fb42cbe32962e77a4741ac1af01/chrome/browser/resources/settings/device_page/night_light_slider.html [modify] https://crrev.com/946fd58e553c2fb42cbe32962e77a4741ac1af01/chrome/test/data/webui/settings/display_size_slider_test.js [modify] https://crrev.com/946fd58e553c2fb42cbe32962e77a4741ac1af01/ui/webui/resources/cr_elements/cr_slider/cr_slider.html [modify] https://crrev.com/946fd58e553c2fb42cbe32962e77a4741ac1af01/ui/webui/resources/cr_elements/cr_slider/cr_slider.js [modify] https://crrev.com/946fd58e553c2fb42cbe32962e77a4741ac1af01/ui/webui/resources/cr_elements/shared_vars_css.html
,
Jun 29 2018
|
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by scottchen@chromium.org
, Jun 6 2018A few notes: 1) I was on js fiddle messing around, and was able to reproduce the pin with just a div and some CSS. FWIW: #marker { width 30px height 30px border-radius 50% 50% 50% 0 background #89849b position absolute transform rotate(-45deg) left 50% top 50% margin -20px 0 0 -20px } 2) I was looking at the existing paper-sliders: https://cs.chromium.org/search/?q=%22%3Cpaper-slider%22+-infra/&sq=package:chromium&type=cs and I'm not sure if the file-manager UI would want this update. Maybe check with UX/ChromeOS UX.