[webkit-reviews] review canceled: [Bug 222990] Web Inspector: `Styles` sidebar pseudo-class checkboxes appear cramped after resizing window at narrow widths : [Attachment 423010] Patch v1.0

[bugzilla-daemon at webkit.org]

Patrick Angle <pangle at apple.com> has canceled Patrick Angle
<pangle at apple.com>'s request for review:
Bug 222990: Web Inspector: `Styles` sidebar pseudo-class checkboxes appear
cramped after resizing window at narrow widths
https://bugs.webkit.org/show_bug.cgi?id=222990

Attachment 423010: Patch v1.0

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




--- Comment #5 from Patrick Angle <pangle at apple.com> ---
Comment on attachment 423010
  --> https://bugs.webkit.org/attachment.cgi?id=423010
Patch v1.0

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

>> Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:177
>> +	    if (!flag && this.isAttached)
> 
> Please use `this.collapsed` instead as that has the fallback `|| false`.
> 
> Also, I think `flag` should be removed altogether because the callsite
doesn't provide any arguments ��

Based on you comment below, this check should probably just be a part of
`_updateMinimumWidthForMultipleSidebars` as well.

>> Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:208
>> +	    if (!this._requiredMinimumWidthForMultipleSidebars &&
this.isAttached)
> 
> Rather than have this here, I think it may be worth inverting this and moving
it to `_updateMinimumWidthForMultipleSidebars` (and probably add a `force`
parameter that will overrule `!this._requiredMinimumWidthForMultipleSidebars`)
so that we don't try to set `_requiredMinimumWidthForMultipleSidebars` until we
know we're ready.

Sounds reasonable to me.

>> Source/WebInspectorUI/UserInterface/Views/SingleSidebar.js:91
>> +		this.selectedSidebarPanel = sidebarPanel;
> 
> this.selectedSidebarPanel ||= sidebarPanel;

How quickly I forget after spending so long working in the backend ��

>> Source/WebInspectorUI/UserInterface/Views/SingleSidebar.js:105
>> +		if (!this.collapsed)
> 
> Why?	I would consider it a bug to try to `set selectedSidebarPanel` while
`collapsed`.

I need to dive deeper on this. I’m not sure I understand the ramifications of
r270134 on this logic, but without this check it was possible to attempt to add
the sub view twice. It seems as if we should be able to set a selected panel
before uncollapsing a sidebar, though. Otherwise we need to wait until the
sidebar is showing to set the selected panel. I’ll look tomorrow.