[webkit-reviews] review granted: [Bug 217396] Web Inspector: Support three-column pane arrangement in Elements Tab : [Attachment 411640] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Oct 19 12:08:36 PDT 2020
Devin Rousso <drousso at apple.com> has granted Patrick Angle <pangle at apple.com>'s
request for review:
Bug 217396: Web Inspector: Support three-column pane arrangement in Elements
Tab
https://bugs.webkit.org/show_bug.cgi?id=217396
Attachment 411640: Patch
https://bugs.webkit.org/attachment.cgi?id=411640&action=review
--- Comment #20 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 411640
--> https://bugs.webkit.org/attachment.cgi?id=411640
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=411640&action=review
r=me, awesome work!
I have a few more style fixes. As a general piece of advice, consider
installing <https://eslint.org> and hooking it up to your editor, as that
should (hopefully) catch many issues before review (note that we don't exactly
follow eslint, such as including extra parenthesis/escapes for clarity). You
may have to slightly change the config to get proper linting for new language
features tho (i think 2018 -> 2020 for optional chaining or logical
assignment).
> Source/WebInspectorUI/UserInterface/Views/MultiSidebar.css:33
> + flex-direction: row-reverse;
clever
> Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:52
> + get primarySidebar() {
Style: `{` on separate line
> Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:56
> + get allowMultipleSidebars() {
ditto (:52)
> Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:70
> + get multipleSidebarsVisible() {
ditto (:52)
> Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:116
> + get selectedSidebarPanel() {
ditto (:52)
> Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:120
> + set selectedSidebarPanel(sidebarPanelOrIdentifierOrIndex) {
ditto (:52)
> Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:124
> + get collapsable() {
ditto (:52)
> Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:144
> + return this.element.offsetWidth;
unnecessary trailing space
> Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:258
> + for (var i = index - 1; i >= 0; i--) {
`let`
> Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:273
> + for (let sidebar of this._sidebars)
Style: we only omit the `{` `}` when the body of the control flow is a single
line
> Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:277
> + for (let sidebar of this._sidebars)
> + if (sidebar.sidebarPanels.includes(sidebarPanel))
> + return sidebar;
> +
> + return null;
I think you can simplify this
```
return this._sidebars.find((sidebar) =>
sidebar.sidebarPanels.includes(sidebarPanel)) || null;
```
> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsSidebarPanel.js:35
> + get allowExclusivePresentation() {
ditto (MultiSidebar.js:52)
> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:398
> + experimentalSettingsView.addSetting(WI.UIString("Elements Tab:",
"Elements Tab: @ Experimental Settings", "Category label for experimental
settings pertaining to the Elements tab"),
WI.settings.experimentalEnableExclusiveDetailsPanels, WI.UIString("Show
independent Styles sidebar"));
NIT: We should have the name of the `WI.ExperimentalSetting` match the name of
the toggle. How about `experimentalEnableIndepdendentStylesPanel`? Generally
speaking it's a good idea to have the names of things in code match the name of
things in the UI (I'd also consider changing `WI.SidebarPanel.prototype.get
exclusive` to `get independent` as well, but that's up to you).
> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:90
> + get selectedSidebarPanel() {
ditto (MultiSidebar.js:52)
> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:112
> + get collapsed() {
ditto (MultiSidebar.js:52)
> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:139
> + get collapsable() {
ditto (MultiSidebar.js:52)
> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:143
> + set collapsable(allow) {
ditto (MultiSidebar.js:52)
> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:168
> + shouldInsertSidebarPanel(sidebarPanel, index) {
ditto (MultiSidebar.js:52)
> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:169
> + /* Implemented by subclasses if needed. */
`//`
> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:173
> + didInsertSidebarPanel(sidebarPanel, index) {
ditto (MultiSidebar.js:52)
> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:174
> + /* Implemented by subclasses if needed. */
ditto (:169)
> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:177
> + didRemoveSidebarPanel(sidebarPanel) {
ditto (MultiSidebar.js:52)
> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:178
> + /* Implemented by subclasses if needed. */
ditto (:169)
> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:181
> + willSetSelectedSidebarPanel(sidebarPanel) {
ditto (MultiSidebar.js:52)
> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:182
> + /* Implemented by subclasses if needed. */
ditto (:169)
> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:185
> + didSetSelectedSidebarPanel(sidebarPanel) {
ditto (MultiSidebar.js:52)
> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:186
> + /* Implemented by subclasses if needed. */
ditto (:169)
> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:189
> + didSetCollapsed(flag) {
ditto (MultiSidebar.js:52)
> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:190
> + /* Implemented by subclasses if needed. */
ditto (:169)
> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:197
> + var sidebarPanel = null;
`let`
> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:205
> + for (var i = 0; i < this._sidebarPanels.length; ++i) {
ditto (:197)
> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:210
> + for (var i = 0; i < this._sidebarPanels.length; ++i) {
> + if (this._sidebarPanels[i].identifier ===
sidebarPanelOrIdentifierOrIndex) {
> + sidebarPanel = this._sidebarPanels[i];
> + break;
> + }
> + }
I think you can simplify this
```
sidebarPanel = this._sidebarPanels.find((existingSidebarPanel) =>
existingSidebarPanel.identifier === sidebarPanelOrIdentifierOrIndex) || null;
```
> Source/WebInspectorUI/UserInterface/Views/SidebarPanel.js:96
> + get exclusive() { return this._exclusive; }
Style: We only inline `get`-`set` pairs when both are inlined. In this case,
since the `set` is multiple lines, please also make the `get` multiple lines.
> Source/WebInspectorUI/UserInterface/Views/SingleSidebar.js:63
> + get width() {
ditto (MultiSidebar.js:52)
> Source/WebInspectorUI/UserInterface/Views/SingleSidebar.js:92
> + {
unnecessary trailing whitespace
> Source/WebInspectorUI/UserInterface/Views/SingleSidebar.js:141
> + var newWidth = positionDelta + this._widthBeforeResize;
`let`
> Source/WebInspectorUI/UserInterface/Views/TabBrowser.js:319
> + assert(false, "panel selection not allowed for navigation
sidebar")
`console.assert`
Also, I'd personally suggest changing this back to the way it was, since we can
assume that `event.target` will never be `this._navigationSidebar` since we
only add this event listener to `this._detailsSidebar` :)
> Source/WebInspectorUI/UserInterface/Views/TabBrowser.js:323
> + if (tabContentView.managesDetailsSidebarPanels)
> + return;
we already did this check above so it doesn't need to be repeated
> Source/WebInspectorUI/UserInterface/Views/TabBrowser.js:327
> +
Style: unnecessary newline
More information about the webkit-reviews
mailing list