Bug 175808 – Web Inspector: Relax the maximum sidebar width

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Matt Baker]

Summary:
Relax the maximum sidebar width.

This patch introduces an arbitrary constant, minimumContentBrowserWidth, which is used to determine the maximum width of a sidebar.

[Radar WebKit Bug Importer]

<rdar://problem/34005339>

[Matt Baker]

Created attachment 318722 [details]
Patch

[Devin Rousso]

Comment on attachment 318722 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Base/Main.js:1053
> +    const minimumContentBrowserWidth = 150;

Should we have a FIXME for making this number more dynamic?

[Matt Baker]

(In reply to Devin Rousso from comment #3)
> Comment on attachment 318722 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=318722&action=review
> 
> > Source/WebInspectorUI/UserInterface/Base/Main.js:1053
> > +    const minimumContentBrowserWidth = 150;
> 
> Should we have a FIXME for making this number more dynamic?

I'd rather not have the minimum change based on the content view, that could be surprising. Instead it would be better to build better collapsing support into our toolbars.

[Devin Rousso]

Comment on attachment 318722 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Base/Main.js:1053
>>> +    const minimumContentBrowserWidth = 150;
>> 
>> Should we have a FIXME for making this number more dynamic?
> 
> I'd rather not have the minimum change based on the content view, that could be surprising. Instead it would be better to build better collapsing support into our toolbars.

OK that sounds good.  A comment explaining how you arrived at 150 would be nice though :)

[Matt Baker]

(In reply to Devin Rousso from comment #5)
> Comment on attachment 318722 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=318722&action=review
> 
> >>> Source/WebInspectorUI/UserInterface/Base/Main.js:1053
> >>> +    const minimumContentBrowserWidth = 150;
> >> 
> >> Should we have a FIXME for making this number more dynamic?
> > 
> > I'd rather not have the minimum change based on the content view, that could be surprising. Instead it would be better to build better collapsing support into our toolbars.
> 
> OK that sounds good.  A comment explaining how you arrived at 150 would be
> nice though :)

So my Changelog comment that this patch introduces an arbitrary constant" isn't sufficient?! I'll include a better explanation.

[Matt Baker]

I'm going to make this a little more sophisticated. We should take into account whether the current TabContentView even supports the other sidebar (the sidebar not being resized).

[Matt Baker]

Created attachment 319030 [details]
[Image] NavigationBar buttons hidden

Our navigation bar implementation wraps items that can't be shown at a given width. It should be possible to ensure that crucial items (such as sidebar show/hide) are always visible.

[Matt Baker]

(In reply to Matt Baker from comment #8)
> Created attachment 319030 [details]
> [Image] NavigationBar buttons hidden
> 
> Our navigation bar implementation wraps items that can't be shown at a given
> width. It should be possible to ensure that crucial items (such as sidebar
> show/hide) are always visible.

Once <https://webkit.org/b/175999> lands it will be safe to go ahead with this change.

[Matt Baker]

Created attachment 319115 [details]
Patch

[Devin Rousso]

Comment on attachment 319115 [details]
Patch

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

r=me

Thanks for adding the ChangeLog comment about 100px.  Makes a lot more sense :)

> Source/WebInspectorUI/UserInterface/Base/Main.js:1066
> +    let otherSidebar;
> +    if (sidebar === this.navigationSidebar)
> +        otherSidebar = tabContentView.detailsSidebarPanels.length ? this.detailsSidebar : null;
> +    else
> +        otherSidebar = tabContentView.navigationSidebarPanel ? this.navigationSidebar : null;

I personally don't like the idea of leaving `let` uninitialized. I think it's easier to read when written like this:

    let otherSidebar = null;
    if (sidebar === this.navigationSidebar && tabContentView.detailsSidebarPanels.length)
        otherSidebar = this.detailsSidebar;
    else if (sidebar === this.detailsSidebar && tabContentView.navigationSidebarPanel)
        otherSidebar = this.navigationSidebar;

[Matt Baker]

Created attachment 319978 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 319978 [details]
Patch

Clearing flags on attachment: 319978

Committed r221713: <http://trac.webkit.org/changeset/221713>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.