[webkit-reviews] review granted: [Bug 193982] Web Inspector: Improve API and documentation of ColumnChart : [Attachment 360500] [PATCH] Proposed Fix

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 193982: Web Inspector: Improve API and documentation of ColumnChart
https://bugs.webkit.org/show_bug.cgi?id=193982

Attachment 360500: [PATCH] Proposed Fix

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




--- Comment #2 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 360500
  --> https://bugs.webkit.org/attachment.cgi?id=360500
[PATCH] Proposed Fix

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

r=me

> Source/WebInspectorUI/ChangeLog:8
> +	   This used to be named "BarChart", convert remaining instances

Grammar: this sentence is a fragment, so use a ";" or add some other
conjunction.

> Source/WebInspectorUI/UserInterface/Views/ColumnChart.js:59
> +    get columns() { return this._columns; }

Considering that nobody calls this (and frankly the data it contains is really
not very useful), I think we should just remove this.

> Source/WebInspectorUI/UserInterface/Views/ColumnChart.js:82
> +	   this._columns = [];

I feel like this should call `this.needsLayout()` 🤔

> Source/WebInspectorUI/UserInterface/Views/ColumnChart.js:85
>      needsLayout()

Perhaps this class should be a subclass of `WI.View`?

> Source/WebInspectorUI/UserInterface/Views/StackedLineChart.js:63
> +    get element() { return this._element; }

Yes! ᕦ(ò_óˇ)ᕤ