Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
526423002:
[DevTools] Combine media queries preview by sections. (Closed)
|
Created:
6 years, 3 months ago by dgozman Modified:
6 years, 3 months ago CC:
blink-reviews, caseq+blink_chromium.org, loislo+blink_chromium.org, eustas+blink_chromium.org, malch+blink_chromium.org, yurys+blink_chromium.org, lushnikov+blink_chromium.org, vsevik+blink_chromium.org, pfeldman+blink_chromium.org, paulirish+reviews_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, sergeyv+blink_chromium.org, aandrey+blink_chromium.org Base URL:
svn://svn.chromium.org/blink/trunk Project:
blink Visibility:
Public. |
Description[DevTools] Combine media queries preview by sections.
MQs in one section now overlay. Hovering will show the values on the ruler.
BUG=409870
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181479
Patch Set 1 #
Total comments: 14
Patch Set 2 : rebased #Patch Set 3 : ruler at the top #Patch Set 4 : removed serifs #Patch Set 5 : fixed comments, less height #
Total comments: 10
Patch Set 6 : fixed review comments #Patch Set 7 : visual tweaks #
Total comments: 4
Patch Set 8 : fixed more review comments #
Messages
Total messages: 19 (6 generated)
dgozman@chromium.org changed reviewers: + lushnikov@chromium.org, pfeldman@chromium.org
Take a look please. Sample video attached to the bug.
https://codereview.chromium.org/526423002/diff/1/Source/devtools/front_end/to... File Source/devtools/front_end/toolbox/MediaQueryInspector.js (right): https://codereview.chromium.org/526423002/diff/1/Source/devtools/front_end/to... Source/devtools/front_end/toolbox/MediaQueryInspector.js:372: if (children[i].containsEventPoint(event)) { can't we use relatedTarget/target instead of this loop? https://codereview.chromium.org/526423002/diff/1/Source/devtools/front_end/to... Source/devtools/front_end/toolbox/MediaQueryInspector.js:388: for (var i = children.length - 1; i >= 0; --i) { Can we simply update z-index of a marker (if any), and in this case setup marker-under-highlighted class for all the children except it? https://codereview.chromium.org/526423002/diff/1/Source/devtools/front_end/to... Source/devtools/front_end/toolbox/MediaQueryInspector.js:436: if (!this._highlightedModel) Can we extract this if-branch in a separate method so that it would be easier to follow? e.g. _renderRulerProjections https://codereview.chromium.org/526423002/diff/1/Source/devtools/front_end/to... Source/devtools/front_end/toolbox/MediaQueryInspector.js:452: if (this._availableSections[i]) availableSections is used only here; lets compute it here as well https://codereview.chromium.org/526423002/diff/1/Source/devtools/front_end/to... Source/devtools/front_end/toolbox/MediaQueryInspector.js:461: if (this._highlightedModel.maxWidthExpression()) { curly braces are not needed https://codereview.chromium.org/526423002/diff/1/Source/devtools/front_end/to... Source/devtools/front_end/toolbox/MediaQueryInspector.js:471: if (this._highlightedModel.maxWidthExpression()) { this if-statment look almost identical to the previous one. can we dedup the code? https://codereview.chromium.org/526423002/diff/1/Source/devtools/front_end/to... Source/devtools/front_end/toolbox/MediaQueryInspector.js:509: window.setImmediate(this._renderRulerDecorations.bind(this)); why do you use setImmediate here? https://codereview.chromium.org/526423002/diff/1/Source/devtools/front_end/to... Source/devtools/front_end/toolbox/MediaQueryInspector.js:532: if (model.maxWidthExpression() && model.minWidthExpression()) { curlies are not needed https://codereview.chromium.org/526423002/diff/1/Source/devtools/front_end/to... Source/devtools/front_end/toolbox/MediaQueryInspector.js:534: } else if (model.maxWidthExpression()) { ditto https://codereview.chromium.org/526423002/diff/1/Source/devtools/front_end/to... Source/devtools/front_end/toolbox/MediaQueryInspector.js:536: } else { ditto
PTAL https://codereview.chromium.org/526423002/diff/1/Source/devtools/front_end/to... File Source/devtools/front_end/toolbox/MediaQueryInspector.js (right): https://codereview.chromium.org/526423002/diff/1/Source/devtools/front_end/to... Source/devtools/front_end/toolbox/MediaQueryInspector.js:436: if (!this._highlightedModel) On 2014/09/03 14:30:41, lushnikov wrote: > Can we extract this if-branch in a separate method so that it would be easier to > follow? e.g. _renderRulerProjections This method is removed now. https://codereview.chromium.org/526423002/diff/1/Source/devtools/front_end/to... Source/devtools/front_end/toolbox/MediaQueryInspector.js:452: if (this._availableSections[i]) On 2014/09/03 14:30:41, lushnikov wrote: > availableSections is used only here; lets compute it here as well Done. https://codereview.chromium.org/526423002/diff/1/Source/devtools/front_end/to... Source/devtools/front_end/toolbox/MediaQueryInspector.js:509: window.setImmediate(this._renderRulerDecorations.bind(this)); On 2014/09/03 14:30:41, lushnikov wrote: > why do you use setImmediate here? It was necessary to have isShowing return the right value. I'd better do this in wasHidden notification, but we don't have such. Anyway, this is removed. https://codereview.chromium.org/526423002/diff/1/Source/devtools/front_end/to... Source/devtools/front_end/toolbox/MediaQueryInspector.js:532: if (model.maxWidthExpression() && model.minWidthExpression()) { On 2014/09/03 14:30:41, lushnikov wrote: > curlies are not needed Done.
https://codereview.chromium.org/526423002/diff/80001/Source/devtools/front_en... File Source/devtools/front_end/responsiveDesignView.css (right): https://codereview.chromium.org/526423002/diff/80001/Source/devtools/front_en... Source/devtools/front_end/responsiveDesignView.css:13: .responsive-design canvas { lets use .responsive-design-canvas selector https://codereview.chromium.org/526423002/diff/80001/Source/devtools/front_en... Source/devtools/front_end/responsiveDesignView.css:536: .media-inspector-marker-max-width .media-inspector-marker-serif { lets use single selector https://codereview.chromium.org/526423002/diff/80001/Source/devtools/front_en... Source/devtools/front_end/responsiveDesignView.css:540: .media-inspector-marker-min-max-width .media-inspector-marker-serif { ditto https://codereview.chromium.org/526423002/diff/80001/Source/devtools/front_en... File Source/devtools/front_end/toolbox/MediaQueryInspector.js (right): https://codereview.chromium.org/526423002/diff/80001/Source/devtools/front_en... Source/devtools/front_end/toolbox/MediaQueryInspector.js:37: WebInspector.MediaQueryInspector.SectionMargin = 2; You don't need to hard-code these css computed style values in the js (see comment below) https://codereview.chromium.org/526423002/diff/80001/Source/devtools/front_en... Source/devtools/front_end/toolbox/MediaQueryInspector.js:76: cssHeight: function() you don't need this method; all clients could simple get offsetHeight of MQI.element https://codereview.chromium.org/526423002/diff/80001/Source/devtools/front_en... Source/devtools/front_end/toolbox/MediaQueryInspector.js:298: container.style.height = WebInspector.MediaQueryInspector.SectionHeight + "px"; your container height depends on font size. You should put some text into container with color: transparent to force it get an appropriate height instead of hard-coding it. This will guarantee that text will always fit nicely into container. https://codereview.chromium.org/526423002/diff/80001/Source/devtools/front_en... File Source/devtools/front_end/toolbox/ResponsiveDesignView.js (right): https://codereview.chromium.org/526423002/diff/80001/Source/devtools/front_en... Source/devtools/front_end/toolbox/ResponsiveDesignView.js:504: this._cachedCssCanvasWidth !== cssCanvasWidth || this._cachedCssCanvasHeight !== cssCanvasHeight || this._cachedZoomFactor !== zoomFactor || let's not wrap this. (this also looks confusing in codereview)
PTAL https://codereview.chromium.org/526423002/diff/80001/Source/devtools/front_en... File Source/devtools/front_end/responsiveDesignView.css (right): https://codereview.chromium.org/526423002/diff/80001/Source/devtools/front_en... Source/devtools/front_end/responsiveDesignView.css:13: .responsive-design canvas { On 2014/09/05 12:48:06, lushnikov wrote: > lets use .responsive-design-canvas selector Done. https://codereview.chromium.org/526423002/diff/80001/Source/devtools/front_en... File Source/devtools/front_end/toolbox/MediaQueryInspector.js (right): https://codereview.chromium.org/526423002/diff/80001/Source/devtools/front_en... Source/devtools/front_end/toolbox/MediaQueryInspector.js:37: WebInspector.MediaQueryInspector.SectionMargin = 2; On 2014/09/05 12:48:06, lushnikov wrote: > You don't need to hard-code these css computed style values in the js (see > comment below) Done. https://codereview.chromium.org/526423002/diff/80001/Source/devtools/front_en... Source/devtools/front_end/toolbox/MediaQueryInspector.js:76: cssHeight: function() On 2014/09/05 12:48:06, lushnikov wrote: > you don't need this method; all clients could simple get offsetHeight of > MQI.element Done. Nice catch, thanks!
lgtm https://codereview.chromium.org/526423002/diff/100002/Source/devtools/front_e... File Source/devtools/front_end/toolbox/MediaQueryInspector.js (right): https://codereview.chromium.org/526423002/diff/100002/Source/devtools/front_e... Source/devtools/front_end/toolbox/MediaQueryInspector.js:280: var bar = this._createElementFromMediaQueryModel(/** @type {!Element} */ (container), marker.model); FYI: I would rename _createElementFromMediaQueryModel into _populateFromMediaQueryModel as far as it not only creates, but also appends the element to the container. https://codereview.chromium.org/526423002/diff/100002/Source/devtools/front_e... Source/devtools/front_end/toolbox/MediaQueryInspector.js:287: if (this.element.children.length != oldChildrenCount) !==
https://codereview.chromium.org/526423002/diff/100002/Source/devtools/front_e... File Source/devtools/front_end/toolbox/MediaQueryInspector.js (right): https://codereview.chromium.org/526423002/diff/100002/Source/devtools/front_e... Source/devtools/front_end/toolbox/MediaQueryInspector.js:280: var bar = this._createElementFromMediaQueryModel(/** @type {!Element} */ (container), marker.model); On 2014/09/05 15:52:04, lushnikov wrote: > FYI: I would rename _createElementFromMediaQueryModel into > _populateFromMediaQueryModel as far as it not only creates, but also appends the > element to the container. Changed to call appendChild right here. https://codereview.chromium.org/526423002/diff/100002/Source/devtools/front_e... Source/devtools/front_end/toolbox/MediaQueryInspector.js:287: if (this.element.children.length != oldChildrenCount) On 2014/09/05 15:52:04, lushnikov wrote: > !== Done.
The CQ bit was checked by dgozman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dgozman@chromium.org/526423002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to request the patch to try. Please note that binary files are still unsupported at the moment, this is being worked on. Thanks for your patience. Transient error: <urlopen error _ssl.c:489: The handshake operation timed out>
The CQ bit was checked by dgozman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dgozman@chromium.org/526423002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/bu...)
The CQ bit was checked by dgozman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dgozman@chromium.org/526423002/120001
Message was sent while issue was closed.
Committed patchset #8 (id:120001) as 181479 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
