Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

misc: minor cleanup of audit-details type names #10603

Merged
merged 3 commits into from
Apr 18, 2020
Merged

misc: minor cleanup of audit-details type names #10603

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3cc1f0c
core(audit-details): use same format for tables and opportunities
brendankenny Apr 17, 2020
68c6e39
remove table rendering canonicalization code
brendankenny Apr 17, 2020
8ebfbc9
misc: minor cleanup of audit-details type names
brendankenny Apr 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions lighthouse-core/report/html/renderer/details-renderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ class DetailsRenderer {
* Render a details item value for embedding in a table. Renders the value
* based on the heading's valueType, unless the value itself has a `type`
* property to override it.
* @param {LH.Audit.Details.Value} value
* @param {LH.Audit.Details.ItemValue} value
* @param {LH.Audit.Details.OpportunityColumnHeading} heading
* @return {Element|null}
*/
Expand Down Expand Up @@ -337,7 +337,7 @@ class DetailsRenderer {
}

/**
* @param {LH.Audit.Details.Value[]} values
* @param {LH.Audit.Details.ItemValue[]} values
* @param {LH.Audit.Details.OpportunityColumnHeading} heading
* @return {Element}
*/
Expand Down
26 changes: 15 additions & 11 deletions types/audit-details.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,11 +81,14 @@ declare global {
[p: string]: any;
}

/** String enum of possible types of values found within table items. */
type ItemValueType = 'bytes' | 'code' | 'link' | 'ms' | 'multi' | 'node' | 'source-location' | 'numeric' | 'text' | 'thumbnail' | 'timespanMs' | 'url';

/** Possible types of values found within table items. */
type ItemValueTypes = 'bytes' | 'code' | 'link' | 'ms' | 'multi' | 'node' | 'source-location' | 'numeric' | 'text' | 'thumbnail' | 'timespanMs' | 'url';
type Value = string | number | boolean | DebugData | NodeValue | SourceLocationValue | LinkValue | UrlValue | CodeValue;
type ItemValue = string | number | boolean | DebugData | NodeValue | SourceLocationValue | LinkValue | UrlValue | CodeValue;

// TODO(bckenny): unify Table/Opportunity headings and items on next breaking change.
// TODO: drop TableColumnHeading, rename OpportunityColumnHeading to TableColumnHeading and
// use that for all table-like audit details.

export interface TableColumnHeading {
/**
Expand All @@ -100,20 +103,20 @@ declare global {
* those values will be primitives rendered as this type, but the values
* could also be objects with their own type to override this field.
*/
itemType: ItemValueTypes;
itemType: ItemValueType;
/**
* Optional - defines an inner table of values that correspond to this column.
* Key is required - if other properties are not provided, the value for the heading is used.
*/
subRows?: {key: string, itemType?: ItemValueTypes, displayUnit?: string, granularity?: number};
subRows?: {key: string, itemType?: ItemValueType, displayUnit?: string, granularity?: number};

displayUnit?: string;
granularity?: number;
}

export type TableItem = {
export interface TableItem {
debugData?: DebugData;
[p: string]: undefined | Value | Value[];
[p: string]: undefined | ItemValue | ItemValue[];
}

export interface OpportunityColumnHeading {
Expand All @@ -129,25 +132,26 @@ declare global {
* those values will be primitives rendered as this type, but the values
* could also be objects with their own type to override this field.
*/
valueType: ItemValueTypes;
valueType: ItemValueType;
/**
* Optional - defines an inner table of values that correspond to this column.
* Key is required - if other properties are not provided, the value for the heading is used.
*/
subRows?: {key: string, valueType?: ItemValueTypes, displayUnit?: string, granularity?: number};
subRows?: {key: string, valueType?: ItemValueType, displayUnit?: string, granularity?: number};

// NOTE: not used by opportunity details, but used in the renderer until table/opportunity unification.
displayUnit?: string;
granularity?: number;
}

export interface OpportunityItem {
/** A more specific table element used for `opportunity` tables. */
export interface OpportunityItem extends TableItem {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OpportunityItem was already a TableItem in tsc's eyes (since it checks using structural typing), this just makes the relationship more explicit. OpportunityItem exists to let opportunity audits know what properties to include on their table items, while the more generic TableItem is what details-renderer knows how to render (regardless of specific keys).

url: string;
wastedBytes?: number;
totalBytes?: number;
wastedMs?: number;
debugData?: DebugData;
[p: string]: undefined | Value | Value[];
[p: string]: undefined | ItemValue | ItemValue[];
}

/**
Expand Down