Bug 193982 – Web Inspector: Improve API and documentation of ColumnChart

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

Improve API and documentation of ColumnChart

Used to be "BarChart" and some remnants of "bar" still existed.

[Joseph Pecoraro]

Created attachment 360500 [details]
[PATCH] Proposed Fix

[Devin Rousso]

Comment on attachment 360500 [details]
[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! ᕦ(ò_óˇ)ᕤ

[Joseph Pecoraro]

(In reply to Devin Rousso from comment #2)
> Comment on attachment 360500 [details]
> [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.

kk.

> > 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.

Okay.

> > Source/WebInspectorUI/UserInterface/Views/ColumnChart.js:85
> >      needsLayout()
> 
> Perhaps this class should be a subclass of `WI.View`?

Will do in a bit.

[Joseph Pecoraro]

https://trac.webkit.org/changeset/240865/webkit

[Radar WebKit Bug Importer]

<rdar://problem/47747238>