Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1917543002:
DevTools: Update styles for sensors drawer panel (Closed)
|
Created:
4 years, 8 months ago by luoe Modified:
4 years, 7 months ago Reviewers:
lushnikov CC:
chromium-reviews, caseq+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, sergeyv+blink_chromium.org, pfeldman, kozyatinskiy+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDevTools: Update styles for sensors drawer panel
BUG=604833
Committed: https://crrev.com/c80c8d6b64756dbd6b3a34cbf2f7f5e6473eed0c
Cr-Commit-Position: refs/heads/master@{#390131}
Patch Set 1 #Patch Set 2 : UI update for sensors panel #
Total comments: 18
Patch Set 3 : Address comments in patch 2 #Patch Set 4 : Removed logic from patch 3 #
Total comments: 16
Patch Set 5 : Address comments in patch 4 #Patch Set 6 : Minor updates #
Messages
Total messages: 20 (8 generated)
luoe@chromium.org changed reviewers: + lushnikov@chromium.org
some intermediate comments https://codereview.chromium.org/1917543002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/emulation/Geolocation.js (right): https://codereview.chromium.org/1917543002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/Geolocation.js:99: WebInspector.Geolocation.Error = { /** @enum {string} */ https://codereview.chromium.org/1917543002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/Geolocation.js:100: None: "", None: "None", https://codereview.chromium.org/1917543002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/emulation/SensorsView.js (right): https://codereview.chromium.org/1917543002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/SensorsView.js:33: _createGeolocationSection: function(geolocation) jsdoc https://codereview.chromium.org/1917543002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/SensorsView.js:35: var _geogroup = this.contentElement.createChild("section", "sensors-group"); unnecessary leading "_" https://codereview.chromium.org/1917543002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/SensorsView.js:37: var _fields = _geogroup.createChild("div", "geo-fields"); unnecessary leading "_" https://codereview.chromium.org/1917543002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/SensorsView.js:213: function roundAngle(angle) jsdoc https://codereview.chromium.org/1917543002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/SensorsView.js:260: this._alphaSetter = this._createAxisInput(cellElement, this._alphaElement, "Tilt left/right (\u03B1)"); We use WebInspector.UIString for every string, presented in the interface https://codereview.chromium.org/1917543002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js (right): https://codereview.chromium.org/1917543002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js:1522: if (input.type !== "number") why do we avoid this for number inputs?
https://codereview.chromium.org/1917543002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/emulation/Geolocation.js (right): https://codereview.chromium.org/1917543002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/Geolocation.js:99: WebInspector.Geolocation.Error = { On 2016/04/23 00:19:46, lushnikov wrote: > /** @enum {string} */ Done. https://codereview.chromium.org/1917543002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/Geolocation.js:100: None: "", On 2016/04/23 00:19:46, lushnikov wrote: > None: "None", Okay. I've also updated another line from: if (this.error) to if (this.error !== WebInspector.Geolocation.Error.None) https://codereview.chromium.org/1917543002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/emulation/SensorsView.js (right): https://codereview.chromium.org/1917543002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/SensorsView.js:33: _createGeolocationSection: function(geolocation) On 2016/04/23 00:19:46, lushnikov wrote: > jsdoc Done. https://codereview.chromium.org/1917543002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/SensorsView.js:35: var _geogroup = this.contentElement.createChild("section", "sensors-group"); On 2016/04/23 00:19:46, lushnikov wrote: > unnecessary leading "_" Done. https://codereview.chromium.org/1917543002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/SensorsView.js:37: var _fields = _geogroup.createChild("div", "geo-fields"); On 2016/04/23 00:19:46, lushnikov wrote: > unnecessary leading "_" Done. https://codereview.chromium.org/1917543002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/SensorsView.js:213: function roundAngle(angle) On 2016/04/23 00:19:46, lushnikov wrote: > jsdoc Done. https://codereview.chromium.org/1917543002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/SensorsView.js:260: this._alphaSetter = this._createAxisInput(cellElement, this._alphaElement, "Tilt left/right (\u03B1)"); On 2016/04/23 00:19:46, lushnikov wrote: > We use WebInspector.UIString for every string, presented in the interface Done. https://codereview.chromium.org/1917543002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js (right): https://codereview.chromium.org/1917543002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js:1522: if (input.type !== "number") On 2016/04/23 00:19:46, lushnikov wrote: > why do we avoid this for number inputs? According to the spec, number inputs are not supposed to support getting or setting selections. It doesn't seem obvious to me, so maybe we should have a comment explaining this? https://developer.mozilla.org/en-US/docs/Web/API/HTMLInputElement/setSelectio... https://bugs.chromium.org/p/chromium/issues/detail?id=324360
Nice! I've played with it and liked it! LGTM % comments, thank you! https://codereview.chromium.org/1917543002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js (right): https://codereview.chromium.org/1917543002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js:1522: if (input.type !== "number") Yeah, tricky! I would leave a comment here, thanks. https://codereview.chromium.org/1917543002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/emulation/Geolocation.js (right): https://codereview.chromium.org/1917543002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/Geolocation.js:47: WebInspector.Geolocation.parseSetting = function(value) lets add jsdoc while we're here https://codereview.chromium.org/1917543002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/Geolocation.js:63: WebInspector.Geolocation.parseUserInput = function(latitudeString, longitudeString, errorStatus) lets add jsdoc while we are here https://codereview.chromium.org/1917543002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/Geolocation.js:99: WebInspector.Geolocation.Error = { i think having the enum is a bit of overkill here. You are never interested in the value of the error in the geolocation object - only in its existance. Looks like bool should be enough https://codereview.chromium.org/1917543002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/emulation/SensorsView.js (right): https://codereview.chromium.org/1917543002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/SensorsView.js:54: for (var j = 0; j < group.length; ++j) { style: omit {} https://codereview.chromium.org/1917543002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/SensorsView.js:62: // Validated input fieldset style: comments should end with "." https://codereview.chromium.org/1917543002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/SensorsView.js:190: _setSelectElementLabel: function(selectElement, labelValue) lets add jsdoc https://codereview.chromium.org/1917543002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/SensorsView.js:222: return Math.round(angle*1e4)/1e4; having 10000 instead of 1e4 is easier to read/less confusing https://codereview.chromium.org/1917543002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/SensorsView.js:480: WebInspector.SensorsView.ShiftDragOrientationSpeed = 20; this is not used
https://codereview.chromium.org/1917543002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js (right): https://codereview.chromium.org/1917543002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js:1522: if (input.type !== "number") On 2016/04/25 23:33:04, lushnikov wrote: > Yeah, tricky! I would leave a comment here, thanks. Done. https://codereview.chromium.org/1917543002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/emulation/Geolocation.js (right): https://codereview.chromium.org/1917543002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/Geolocation.js:47: WebInspector.Geolocation.parseSetting = function(value) On 2016/04/25 23:33:04, lushnikov wrote: > lets add jsdoc while we're here Done. https://codereview.chromium.org/1917543002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/Geolocation.js:63: WebInspector.Geolocation.parseUserInput = function(latitudeString, longitudeString, errorStatus) On 2016/04/25 23:33:04, lushnikov wrote: > lets add jsdoc while we are here Done. https://codereview.chromium.org/1917543002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/Geolocation.js:99: WebInspector.Geolocation.Error = { On 2016/04/25 23:33:04, lushnikov wrote: > i think having the enum is a bit of overkill here. You are never interested in > the value of the error in the > geolocation object - only in its existance. > > Looks like bool should be enough Done. https://codereview.chromium.org/1917543002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/emulation/SensorsView.js (right): https://codereview.chromium.org/1917543002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/SensorsView.js:54: for (var j = 0; j < group.length; ++j) { On 2016/04/25 23:33:04, lushnikov wrote: > style: omit {} Done. https://codereview.chromium.org/1917543002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/SensorsView.js:62: // Validated input fieldset On 2016/04/25 23:33:04, lushnikov wrote: > style: comments should end with "." Done. https://codereview.chromium.org/1917543002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/SensorsView.js:190: _setSelectElementLabel: function(selectElement, labelValue) On 2016/04/25 23:33:04, lushnikov wrote: > lets add jsdoc Done. https://codereview.chromium.org/1917543002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/SensorsView.js:222: return Math.round(angle*1e4)/1e4; On 2016/04/25 23:33:04, lushnikov wrote: > having 10000 instead of 1e4 is easier to read/less confusing Done. https://codereview.chromium.org/1917543002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/SensorsView.js:480: WebInspector.SensorsView.ShiftDragOrientationSpeed = 20; On 2016/04/25 23:33:04, lushnikov wrote: > this is not used Done. Thanks, I forgot to pull it out. It might appear in a future CL.
The CQ bit was checked by luoe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lushnikov@chromium.org Link to the patchset: https://codereview.chromium.org/1917543002/#ps100001 (title: "Minor updates")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1917543002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1917543002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
The CQ bit was checked by luoe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1917543002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1917543002/100001
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 luoe@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1917543002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1917543002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/c80c8d6b64756dbd6b3a34cbf2f7f5e6473eed0c Cr-Commit-Position: refs/heads/master@{#390131}
Message was sent while issue was closed.
Description was changed from ========== DevTools: Update styles for sensors drawer panel BUG=604833 ========== to ========== DevTools: Update styles for sensors drawer panel BUG=604833 Committed: https://crrev.com/c80c8d6b64756dbd6b3a34cbf2f7f5e6473eed0c Cr-Commit-Position: refs/heads/master@{#390131} ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
