Summary: | Web Inspector: TabBar redesign: improvements to tab layout and resize behavior | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Matt Baker <mattbaker> | ||||||||
Component: | Web Inspector | Assignee: | Matt Baker <mattbaker> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | agomez, commit-queue, ews-watchlist, hi, inspector-bugzilla-changes, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Bug Depends on: | 181448 | ||||||||||
Bug Blocks: | 181611 | ||||||||||
Attachments: |
|
Description
Matt Baker
2018-01-09 18:59:14 PST
As well as: - When one or more tabs are hidden, add a ">>" TabBarItem that when clicked shows a popup menu with a list of hidden tabs. Created attachment 332408 [details]
Patch
Created attachment 332410 [details]
[Video] Resize and tab picker behavior
Comment on attachment 332408 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=332408&action=review r=me, with some Style and NIT > Source/WebInspectorUI/UserInterface/Views/GeneralTabBarItem.js:57 > + get title() > + { > + return super.title; > + } Is this necessary? > Source/WebInspectorUI/UserInterface/Views/GeneralTabBarItem.js:89 > + contextMenu.appendItem(WI.UIString("Close Tab"), () => this._parentTabBar.removeTabBarItem(this), this.isDefaultTab); I think our style is to usually always have the {} for inline functions on ContextMenu items, as we never want to return any value. contextMenu.appendItem(WI.UIString("Close Tab"), () => { this._parentTabBar.removeTabBarItem(this); }, this.isDefaultTab); > Source/WebInspectorUI/UserInterface/Views/TabBar.js:410 > + item.element.classList.toggle("hidden", hidden); One gotcha to keep an eye out for is that if `undefined` is passed as the second argument to `classList.toggle` then it will just flip-flop the class like a lightswitch. You might want to add a `!!` in front of `hidden` so that this can't happen: item.element.classList.toggle("hidden", !!hidden); > Source/WebInspectorUI/UserInterface/Views/TabBar.js:427 > + } Style: missing semicolon > Source/WebInspectorUI/UserInterface/Views/TabBar.js:444 > Style: unnecessary newline > Source/WebInspectorUI/UserInterface/Views/TabBar.js:609 > + contextMenu.appendItem(item.title, () => this.selectedTabBarItem = item); This reads somewhat poorly. I think it would be clearer to not make this one line: for (let item of this._hiddenTabBarItems) { contextMenu.append(item.title, () => { this.selectedTabBarItem = item; }); } Also, does this not need to call `needsLayout()` as it does in `_handleTabPickerTabContextMenu`? Comment on attachment 332408 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=332408&action=review >> Source/WebInspectorUI/UserInterface/Views/GeneralTabBarItem.js:57 >> + } > > Is this necessary? Overriding a property's setter (or getter) shadows the whole property: class Foo { get x() { return 1; } } class Bar extends Foo { set x() {} } > let o = new Bar; > o.x < undefined >> Source/WebInspectorUI/UserInterface/Views/TabBar.js:410 >> + item.element.classList.toggle("hidden", hidden); > > One gotcha to keep an eye out for is that if `undefined` is passed as the second argument to `classList.toggle` then it will just flip-flop the class like a lightswitch. You might want to add a `!!` in front of `hidden` so that this can't happen: > > item.element.classList.toggle("hidden", !!hidden); In practice the second argument is always defined, but it's a good habit to be in. Will change. >> Source/WebInspectorUI/UserInterface/Views/TabBar.js:609 >> + contextMenu.appendItem(item.title, () => this.selectedTabBarItem = item); > > This reads somewhat poorly. I think it would be clearer to not make this one line: > > for (let item of this._hiddenTabBarItems) { > contextMenu.append(item.title, () => { > this.selectedTabBarItem = item; > }); > } > > Also, does this not need to call `needsLayout()` as it does in `_handleTabPickerTabContextMenu`? The layout call in _handleTabPickerTabContextMenu shouldn't be necessary. Will remove. Created attachment 332423 [details]
Patch for landing
Comment on attachment 332423 [details] Patch for landing Clearing flags on attachment: 332423 Committed r227703: <https://trac.webkit.org/changeset/227703> All reviewed patches have been landed. Closing bug. |