[webkit-reviews] review granted: [Bug 200891] Web Inspector: Sources: move the resource type scope bar to be next to the filter : [Attachment 376698] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Devin Rousso
<drousso at apple.com>'s request for review:
Bug 200891: Web Inspector: Sources: move the resource type scope bar to be next
to the filter
https://bugs.webkit.org/show_bug.cgi?id=200891

Attachment 376698: Patch

https://bugs.webkit.org/attachment.cgi?id=376698&action=review




--- Comment #3 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 376698
  --> https://bugs.webkit.org/attachment.cgi?id=376698
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=376698&action=review

Nice! r=me

• I think we should be able to eliminate the "Group By" label
• I think we will want to better handle filtering at the bottom if a long name
filter like "Documents" is selected. Perhaps hiding the fields when clicking
into the filter field.

>
Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:209
> +	   this._resourceGroupingModeNavigationBar = new WI.NavigationBar;

It still seems like `resourcesNavigationBar` is apt. It only has 1 thing in it
right now, but it is still the navigation bar over the resource section.

>
Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:222
> +	   let resourceGroupingModeScopeBarItems =
Object.values(this._resourceGroupingModeScopeBarItems);
> +	   resourceGroupingModeScopeBarItems.unshift(new
WI.ScopeBarItem("sources-resource-grouping-mode-label", WI.UIString("Group
by:"), {disabled: true, exclusive: true}));

I tend to avoid unshift when possible (it can be slow). Here it doesn't seem
necessary and you can avoid the Object.values allocation all-together without
any significant readability loss.

    let items = [];
    items.push(new WI.ScopeBarItem("sources-resource-grouping-mode-label",
WI.UIString("Group by:"), {disabled: true, exclusive: true}));
    for (let key in this._resourceGroupingModeScopeBarItems)
	items.push(this._resourceGroupingModeScopeBarItems[key]);

Up to you.

---

I'd really love to eliminate this `Group by` label. It already looks funky and
it is weird to have so many `.disabled` styles that apply to just it when it
could just be a label in the NavigationBar.

Is just `( Type ) ( Path )` really that bad?

  * Xcode has `( Hierarchical ) ( Flat )` in its Symbol navigator.
  * Xcode has `( Buildtime ) ( Runtime )` in its Issue navigator.
  * Xcode has `( By Group ) ( By Time )` in its Report navigator.
  * Finder / Mail have `Search: ( This Mac ) ( "Foo" )` in their find window
  * Other places (Safari) use segmented controls

I kind of like the idea of `( By Type ) ( By Path )`. But I think just `( Type
) ( Path )` are good enough!