UI Request: Create specs for Display Size Setting |
|||||||||
Issue descriptionPlease create specs for Display Size setting as depicted in Mocks below. Launch Bug: crbug/790723 PRD: go/CrosResolution Mocks: go/DisplaySizeMocks
,
Feb 6 2018
High fidelity mocks have been aded here: https://gallery.googleplex.com/projects/MCHbtQVoQ2HCZZ9oo1_la-_D/files/MCG17nfZaSnyAfyJc1bJy4PLwMSJbxAVNpw Since this is only using existing components, I didn't go through the exercise or writing a spec. Please note that for the scale tooltip, we should use the same type of tooltips used further down in the slider for Night Light settings. Let me know if you need anything else or more details.
,
Feb 6 2018
Malay, please use latest mirroring mocks with check box instead of toggle.
,
Feb 26 2018
#2 The night light does not have a tool tip right now. There is something similar for the schedule slider though. This however is not a tool tip, instead a permanent indicator that hovers above the point.
,
Feb 26 2018
Right I think the idea is to take that, hovering over the point but only when manipulated. Called it tooltip for lack of better word but it's essentially on on-press floating label.
,
Feb 27 2018
@Steven For the above requirement, do you think its a good idea to add this to the <settings-slider> component? This way it can be used system wide consistently.
,
Feb 27 2018
settings-slider should probably have been named pref-slider. Unless this is something that should apply to a slider represing a single pref value, I would not add it there. If we use the behavior in more than one slider, it may make sense to create a different shared component however.
,
Feb 27 2018
It is representing a single value.
,
Feb 27 2018
Is it a single pref store value? And is the behavior something we want in other settings-slider instances?
,
Feb 27 2018
- Is it a single pref store value? Yes - Is the behavior something we want in other settings-slider instances? Looking at the codebase, none of the other usage for setting slider displays the slider value. So for now this will be used for this particular instance.
,
Feb 27 2018
I could see this behavior being useful in other instances like font size settings for example. Pretty much every time a slider doesn't display all stop-gap values.
,
Feb 27 2018
OK, taking another look at settings-slider, there's a fair bit of customization there. Adding this feature seems reasonable.
,
Feb 28 2018
something else that would be helpful for this: show the effective resolution: I always have to go to https://www.whatismyscreenresolution.com on any device to see the effective resolution, but macOS display settings already shows the effective resolution. I think users would appreciate this and it would help them to better understand what is going and and how it works
,
Feb 28 2018
#12 Looking at settings-slider again, I am not sure how to proceed with implementing it. I would need 2 pieces of information to make it work. 1) Detecting mouse enter and leave. paper-tooltip has some reference on how this can be done. 2) Getting the current x position of the slider knob. The slider knob is in the shodow DOM of paper-slider, a third party library. Accessing its elements would be very hacky and prone to failures if there are any upstream changes in how paper-slider is implemented. +michaelpg as well.
,
Feb 28 2018
I don't have any easy answers. +hcarmona@, +dpapad@ who might have some thoughts here, but any such changes should be tested to prevent breakage due to Polymer element updates. The paper elements are mostly comprised of behaviors so it may be easier to create a new element using the same behaviors. This would be a good example where we should get UX to spec out how these sliders should behave, with a rule for when the tooltip-like behavior should appear (i.e. automatically based on the range, or manually by adding a custom attribute). Then we can build the element and sue it throughout Settings. Then the current settings-slider element could be built with the new element (since not all sliders represent pref values).
,
Mar 1 2018
+namraatakannan @malaykeshav: Peeking into the shadow DOM of paper-slider is a no-go. Parent elements should interact with child elements via properly defined APIs (like public methods, or HTML attributes, or CSS mixins). Also note that paper-slider already has a way to display the current value (see demo at [1]). I would be hesitant of implementing a different way, while still leveraging paper-slider under the cover. My suggestion is to have UX figure out what do we want sliders to look like, especially within the context of the upcoming MD2 updates, and implement those as part of that effort. [1] https://www.webcomponents.org/element/PolymerElements/paper-slider/demo/demo/index.html
,
Mar 1 2018
,
Mar 9 2018
,
Mar 24 2018
@sgabriel Attaching a video of the implementation for Display Slider. This includes all operations required from a slider. Do we want to add a special indicator for the default value?
,
Mar 26 2018
This is cool. Thanks Malay. For the default state it would make sent to make it "Default:xx%" if it can be anything else than 100%.
,
Mar 26 2018
The default value is always 100%. However this value will not always have the same position on the slider. A correct way to put this would be, should we change the marker on the slider for the default value? A bigger marker? Or a different color?
,
Mar 29 2018
Recordings of the final implementation sent for review.
,
Mar 30 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8926b625fd56c9b3f418bc41843f6e4b5711baba commit 8926b625fd56c9b3f418bc41843f6e4b5711baba Author: Malay Keshav <malaykeshav@chromium.org> Date: Fri Mar 30 03:30:23 2018 Implements a custom slider for display size setting This patch implements a custom slider that provides the basic functionality of the paper-slider but provides an additional option to display a label for the current slider value. The slider has the Prefs Behavior integrated with it. This slider has a general implemention and is ready to be used in other locations as well. Functionalities implemented: - Slider works the same way as the paper-slider in terms of mouse and keyboard interactions. - Additionally The slider displays a label on mouse hover and keyboard focus. - The slider only works for discrete values or options. This patch also adds web ui tests for all the slider functionalities. A video of this slider in use can be found on the bug link below. Bug: 798906 Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation Change-Id: I55fa98eb438ed1c48b4f40c14d8c05085e7a45b4 Component: Display Slider, display settings, webui tests Reviewed-on: https://chromium-review.googlesource.com/985127 Commit-Queue: Malay Keshav <malaykeshav@chromium.org> Reviewed-by: Steven Bennetts <stevenjb@chromium.org> Cr-Commit-Position: refs/heads/master@{#547101} [modify] https://crrev.com/8926b625fd56c9b3f418bc41843f6e4b5711baba/chrome/app/settings_strings.grdp [modify] https://crrev.com/8926b625fd56c9b3f418bc41843f6e4b5711baba/chrome/browser/resources/settings/device_page/compiled_resources2.gyp [modify] https://crrev.com/8926b625fd56c9b3f418bc41843f6e4b5711baba/chrome/browser/resources/settings/device_page/display.html [modify] https://crrev.com/8926b625fd56c9b3f418bc41843f6e4b5711baba/chrome/browser/resources/settings/device_page/display.js [add] https://crrev.com/8926b625fd56c9b3f418bc41843f6e4b5711baba/chrome/browser/resources/settings/device_page/display_size_slider.html [add] https://crrev.com/8926b625fd56c9b3f418bc41843f6e4b5711baba/chrome/browser/resources/settings/device_page/display_size_slider.js [modify] https://crrev.com/8926b625fd56c9b3f418bc41843f6e4b5711baba/chrome/browser/resources/settings/settings_resources.grd [modify] https://crrev.com/8926b625fd56c9b3f418bc41843f6e4b5711baba/chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc [modify] https://crrev.com/8926b625fd56c9b3f418bc41843f6e4b5711baba/chrome/test/data/webui/settings/cr_settings_browsertest.js [add] https://crrev.com/8926b625fd56c9b3f418bc41843f6e4b5711baba/chrome/test/data/webui/settings/display_size_slider_test.js
,
Mar 30 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9bb4e7ab81af5be33cc71c1127bcd757140d8a1b commit 9bb4e7ab81af5be33cc71c1127bcd757140d8a1b Author: Sebastien Seguin-Gagnon <sebsg@chromium.org> Date: Fri Mar 30 15:49:10 2018 Revert "Implements a custom slider for display size setting" This reverts commit 8926b625fd56c9b3f418bc41843f6e4b5711baba. Reason for revert: Possible culprit for the failing of CrSettingsDevicePageTest.DisplayTest CrSettingsDevicePageTest.KeyboardTest CrSettingsDevicePageTest.PowerTest CrSettingsDisplaySizeSliderTest.All CrSettingsDevicePageTest.DevicePageTest CrSettingsDevicePageTest.PointersTest CrSettingsDevicePageTest.StylusTest on the bots. First build with errors:https://uberchromegw.corp.google.com/i/chromium.chromiumos/builders/linux-chromeos-dbg/builds/4954 9 builds failed in a row with those errors. Original change's description: > Implements a custom slider for display size setting > > This patch implements a custom slider that provides the basic > functionality of the paper-slider but provides an additional option to > display a label for the current slider value. The slider has the Prefs > Behavior integrated with it. > > This slider has a general implemention and is ready to be used in other > locations as well. > > Functionalities implemented: > - Slider works the same way as the paper-slider in terms of mouse and > keyboard interactions. > - Additionally The slider displays a label on mouse hover and keyboard > focus. > - The slider only works for discrete values or options. > > This patch also adds web ui tests for all the slider functionalities. > > A video of this slider in use can be found on the bug link below. > > Bug: 798906 > Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation > Change-Id: I55fa98eb438ed1c48b4f40c14d8c05085e7a45b4 > Component: Display Slider, display settings, webui tests > Reviewed-on: https://chromium-review.googlesource.com/985127 > Commit-Queue: Malay Keshav <malaykeshav@chromium.org> > Reviewed-by: Steven Bennetts <stevenjb@chromium.org> > Cr-Commit-Position: refs/heads/master@{#547101} TBR=stevenjb@chromium.org,malaykeshav@chromium.org Change-Id: I456977e7b8358e6210fc72baf66bc692514f6f27 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 798906 Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation Reviewed-on: https://chromium-review.googlesource.com/988092 Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org> Commit-Queue: Sebastien Seguin-Gagnon <sebsg@chromium.org> Cr-Commit-Position: refs/heads/master@{#547172} [modify] https://crrev.com/9bb4e7ab81af5be33cc71c1127bcd757140d8a1b/chrome/app/settings_strings.grdp [modify] https://crrev.com/9bb4e7ab81af5be33cc71c1127bcd757140d8a1b/chrome/browser/resources/settings/device_page/compiled_resources2.gyp [modify] https://crrev.com/9bb4e7ab81af5be33cc71c1127bcd757140d8a1b/chrome/browser/resources/settings/device_page/display.html [modify] https://crrev.com/9bb4e7ab81af5be33cc71c1127bcd757140d8a1b/chrome/browser/resources/settings/device_page/display.js [delete] https://crrev.com/a82bdb76536b5fce185734225128ca3ccb5a2e8c/chrome/browser/resources/settings/device_page/display_size_slider.html [delete] https://crrev.com/a82bdb76536b5fce185734225128ca3ccb5a2e8c/chrome/browser/resources/settings/device_page/display_size_slider.js [modify] https://crrev.com/9bb4e7ab81af5be33cc71c1127bcd757140d8a1b/chrome/browser/resources/settings/settings_resources.grd [modify] https://crrev.com/9bb4e7ab81af5be33cc71c1127bcd757140d8a1b/chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc [modify] https://crrev.com/9bb4e7ab81af5be33cc71c1127bcd757140d8a1b/chrome/test/data/webui/settings/cr_settings_browsertest.js [delete] https://crrev.com/a82bdb76536b5fce185734225128ca3ccb5a2e8c/chrome/test/data/webui/settings/display_size_slider_test.js
,
Mar 31 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f36c74c4c75c6c7121a013f220f463baffbc7aba commit f36c74c4c75c6c7121a013f220f463baffbc7aba Author: Malay Keshav <malaykeshav@chromium.org> Date: Sat Mar 31 01:53:03 2018 Reland: Implements a custom slider for display size setting This patch implements a custom slider that provides the basic functionality of the paper-slider but provides an additional option to display a label for the current slider value. The slider has the Prefs Behavior integrated with it. This slider has a general implemention and is ready to be used in other locations as well. Functionalities implemented: - Slider works the same way as the paper-slider in terms of mouse and keyboard interactions. - Additionally The slider displays a label on mouse hover and keyboard focus. - The slider only works for discrete values or options. This patch also adds web ui tests for all the slider functionalities. A video of this slider in use can be found on the bug link below. Bug: 798906 Change-Id: Ib51ec47d0d2e8ad83ea66220ddf746fe15208d5f Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation Component: Display Slider, display settings, webui tests Reverted Patch: https://chromium-review.googlesource.com/c/chromium/src/+/988092 TBR=stevenjb@chromium.org Change-Id: Ib51ec47d0d2e8ad83ea66220ddf746fe15208d5f Reviewed-on: https://chromium-review.googlesource.com/988823 Reviewed-by: Malay Keshav <malaykeshav@chromium.org> Commit-Queue: Malay Keshav <malaykeshav@chromium.org> Cr-Commit-Position: refs/heads/master@{#547359} [modify] https://crrev.com/f36c74c4c75c6c7121a013f220f463baffbc7aba/chrome/app/settings_strings.grdp [modify] https://crrev.com/f36c74c4c75c6c7121a013f220f463baffbc7aba/chrome/browser/resources/settings/device_page/compiled_resources2.gyp [modify] https://crrev.com/f36c74c4c75c6c7121a013f220f463baffbc7aba/chrome/browser/resources/settings/device_page/display.html [modify] https://crrev.com/f36c74c4c75c6c7121a013f220f463baffbc7aba/chrome/browser/resources/settings/device_page/display.js [add] https://crrev.com/f36c74c4c75c6c7121a013f220f463baffbc7aba/chrome/browser/resources/settings/device_page/display_size_slider.html [add] https://crrev.com/f36c74c4c75c6c7121a013f220f463baffbc7aba/chrome/browser/resources/settings/device_page/display_size_slider.js [modify] https://crrev.com/f36c74c4c75c6c7121a013f220f463baffbc7aba/chrome/browser/resources/settings/settings_resources.grd [modify] https://crrev.com/f36c74c4c75c6c7121a013f220f463baffbc7aba/chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc [modify] https://crrev.com/f36c74c4c75c6c7121a013f220f463baffbc7aba/chrome/test/data/webui/settings/cr_settings_browsertest.js [add] https://crrev.com/f36c74c4c75c6c7121a013f220f463baffbc7aba/chrome/test/data/webui/settings/display_size_slider_test.js
,
Jul 25
Is this bug fixed now?
,
Jul 25
Yes. This should be marked as fixed. |
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by osh...@chromium.org
, Jan 26 2018