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.

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

report(details-renderer): support sub-rows within a table #10084

Merged
merged 10 commits into from
Dec 17, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
84 changes: 67 additions & 17 deletions lighthouse-core/report/html/renderer/details-renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,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.TableItem[string] | LH.Audit.Details.OpportunityItem[string]} value
* @param {LH.Audit.Details.Value} value
* @param {LH.Audit.Details.OpportunityColumnHeading} heading
* @return {Element|null}
*/
Expand Down Expand Up @@ -246,7 +246,7 @@ class DetailsRenderer {
switch (heading.valueType) {
case 'bytes': {
const numValue = Number(value);
return this._renderBytes({value: numValue, granularity: 1});
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is unrelated to multi, right? just needed for the eventual js audits?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah I noticed it when I was making the js audits. I could pull this out as a one line change. It seems like a mistake to me.

return this._renderBytes({value: numValue, granularity: heading.granularity});
}
case 'code': {
const strValue = String(value);
Expand Down Expand Up @@ -296,22 +296,54 @@ class DetailsRenderer {
* OpportunityColumnHeading type until we have all details use the same
* heading format.
* @param {LH.Audit.Details.Table|LH.Audit.Details.Opportunity} tableLike
* @return {Array<LH.Audit.Details.OpportunityColumnHeading>} header
* @return {Array<LH.Audit.Details.OpportunityColumnHeading>}
*/
_getCanonicalizedTableHeadings(tableLike) {
_getCanonicalizedHeadingsFromTable(tableLike) {
if (tableLike.type === 'opportunity') {
return tableLike.headings;
}

return tableLike.headings.map(heading => {
return {
key: heading.key,
label: heading.text,
valueType: heading.itemType,
displayUnit: heading.displayUnit,
granularity: heading.granularity,
};
});
return tableLike.headings.map(heading => this._getCanonicalizedHeading(heading));
}

/**
* Get the headings of a table-like details object, converted into the
* OpportunityColumnHeading type until we have all details use the same
* heading format.
* @param {LH.Audit.Details.TableColumnHeading} heading
* @return {LH.Audit.Details.OpportunityColumnHeading}
*/
_getCanonicalizedHeading(heading) {
let subRows;
if (heading.subRows) {
// @ts-ignore: It's ok that there is no text.
subRows = this._getCanonicalizedHeading(heading.subRows);
}

return {
key: heading.key,
valueType: heading.itemType,
subRows,
label: heading.text,
displayUnit: heading.displayUnit,
granularity: heading.granularity,
};
}

/**
* @param {LH.Audit.Details.Value[]} values
* @param {LH.Audit.Details.OpportunityColumnHeading} heading
* @return {Element}
*/
_renderSubRows(values, heading) {
const subRowsElement = this._dom.createElement('div', 'lh-sub-rows');
for (const childValue of values) {
const subRowElement = this._renderTableValue(childValue, heading);
if (!subRowElement) continue;
subRowElement.classList.add('lh-sub-row');
subRowsElement.appendChild(subRowElement);
}
return subRowsElement;
}

/**
Expand All @@ -325,7 +357,7 @@ class DetailsRenderer {
const theadElem = this._dom.createChildOf(tableElem, 'thead');
const theadTrElem = this._dom.createChildOf(theadElem, 'tr');

const headings = this._getCanonicalizedTableHeadings(details);
const headings = this._getCanonicalizedHeadingsFromTable(details);

for (const heading of headings) {
const valueType = heading.valueType || 'text';
Expand All @@ -339,12 +371,30 @@ class DetailsRenderer {
for (const row of details.items) {
const rowElem = this._dom.createChildOf(tbodyElem, 'tr');
for (const heading of headings) {
const valueFragment = this._dom.createFragment();

const value = row[heading.key];
const valueElement = this._renderTableValue(value, heading);
const valueElement =
value !== undefined && !Array.isArray(value) && this._renderTableValue(value, heading);
if (valueElement) valueFragment.appendChild(valueElement);

if (heading.subRows) {
const subRowsHeading = {
key: heading.subRows.key,
valueType: heading.subRows.valueType || heading.valueType,
granularity: heading.subRows.granularity || heading.granularity,
displayUnit: heading.subRows.displayUnit || heading.displayUnit,
label: '',
};
const values = row[subRowsHeading.key];
if (!Array.isArray(values)) continue;
const subRowsElement = this._renderSubRows(values, subRowsHeading);
valueFragment.appendChild(subRowsElement);
}

if (valueElement) {
if (valueFragment.childElementCount) {
const classes = `lh-table-column--${heading.valueType}`;
this._dom.createChildOf(rowElem, 'td', classes).appendChild(valueElement);
this._dom.createChildOf(rowElem, 'td', classes).appendChild(valueFragment);
} else {
this._dom.createChildOf(rowElem, 'td', 'lh-table-column--empty');
}
Expand Down
6 changes: 6 additions & 0 deletions lighthouse-core/report/html/report-styles.css
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,7 @@
/* Pallete */
--color-gray-200: var(--color-gray-800);
--color-gray-400: var(--color-gray-600);
--color-gray-700: var(--color-gray-400);
--color-gray-50: #757575;
--color-gray-600: var(--color-gray-500);
--color-green-700: var(--color-green);
Expand Down Expand Up @@ -1405,6 +1406,11 @@
text-decoration: underline dotted #999;
}

.lh-sub-rows:not(:first-child) .lh-sub-row {
margin-left: 20px;
color: var(--color-gray-700);
}

/* Chevron
https://codepen.io/paulirish/pen/LmzEmK
*/
Expand Down
77 changes: 75 additions & 2 deletions lighthouse-core/test/report/html/renderer/details-renderer-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -548,7 +548,7 @@ describe('DetailsRenderer', () => {
// itemType is overriden by code object
headings: [{key: 'content', itemType: 'url', text: 'Heading'}],
items: [
{content: {type: 'code', value: 'code object'}},
{content: {type: 'code', value: 'https://codeobject.com'}},
Copy link
Collaborator Author

@connorjclark connorjclark Dec 9, 2019

Choose a reason for hiding this comment

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

The fallback code in the details render will render a URL w/ an invalid url as a code type - so, this test could possible have a false positive (if we ever refactored and broke the fallback type mechanism). This is avoided by using a valid URL as the value.

{content: 'https://example.com'},
],
};
Expand All @@ -560,7 +560,7 @@ describe('DetailsRenderer', () => {
const codeEl = itemElements[0].firstChild;
assert.equal(codeEl.localName, 'pre');
assert.ok(codeEl.classList.contains('lh-code'));
assert.equal(codeEl.textContent, 'code object');
assert.equal(codeEl.textContent, 'https://codeobject.com');

// Second item uses the heading's specified type for the column.
const urlEl = itemElements[1].firstChild;
Expand All @@ -569,5 +569,78 @@ describe('DetailsRenderer', () => {
assert.equal(urlEl.title, 'https://example.com');
assert.equal(urlEl.textContent, 'https://example.com');
});

describe('subRows', () => {
it('renders', () => {
const details = {
type: 'table',
headings: [{key: 'url', itemType: 'url', subRows: {key: 'sources', itemType: 'code'}}],
items: [
{url: 'https://www.example.com', sources: ['a', 'b', 'c']},
],
};

const el = renderer.render(details);
const columnElement = el.querySelector('td.lh-table-column--url');

// First element is the url.
const codeEl = columnElement.firstChild;
assert.equal(codeEl.localName, 'div');
assert.ok(codeEl.classList.contains('lh-text__url'));
assert.equal(codeEl.textContent, 'https://www.example.com');

// Second element lists the multiple values.
const subRowsEl = columnElement.children[1];
assert.equal(subRowsEl.localName, 'div');
assert.ok(subRowsEl.classList.contains('lh-sub-rows'));

const multiValueEls = subRowsEl.querySelectorAll('.lh-sub-row');
assert.equal(multiValueEls[0].textContent, 'a');
assert.ok(multiValueEls[0].classList.contains('lh-code'));
assert.equal(multiValueEls[1].textContent, 'b');
assert.ok(multiValueEls[1].classList.contains('lh-code'));
assert.equal(multiValueEls[2].textContent, 'c');
assert.ok(multiValueEls[2].classList.contains('lh-code'));
});

it('renders, uses heading properties as fallback', () => {
const details = {
type: 'table',
headings: [{key: 'url', itemType: 'url', subRows: {key: 'sources'}}],
items: [
{
url: 'https://www.example.com',
sources: [
'https://www.a.com',
{type: 'code', value: 'https://www.b.com'},
'https://www.c.com',
],
},
],
};

const el = renderer.render(details);
const columnElement = el.querySelector('td.lh-table-column--url');

// First element is the url.
const codeEl = columnElement.firstChild;
assert.equal(codeEl.localName, 'div');
assert.ok(codeEl.classList.contains('lh-text__url'));
assert.equal(codeEl.textContent, 'https://www.example.com');

// Second element lists the multiple values.
const subRowsEl = columnElement.children[1];
assert.equal(subRowsEl.localName, 'div');
assert.ok(subRowsEl.classList.contains('lh-sub-rows'));

const multiValueEls = subRowsEl.querySelectorAll('.lh-sub-row');
assert.equal(multiValueEls[0].textContent, 'https://www.a.com');
assert.ok(multiValueEls[0].classList.contains('lh-text__url'));
assert.equal(multiValueEls[1].textContent, 'https://www.b.com');
assert.ok(multiValueEls[1].classList.contains('lh-code'));
assert.equal(multiValueEls[2].textContent, 'https://www.c.com');
assert.ok(multiValueEls[2].classList.contains('lh-text__url'));
});
});
});
});
17 changes: 14 additions & 3 deletions types/audit-details.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,8 @@ declare global {
}

/** Possible types of values found within table items. */
type ItemValueTypes = 'bytes' | 'code' | 'link' | 'ms' | 'node' | 'source-location' | 'numeric' | 'text' | 'thumbnail' | 'timespanMs' | 'url';
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;

// TODO(bckenny): unify Table/Opportunity headings and items on next breaking change.

Expand All @@ -97,14 +98,19 @@ declare global {
* could also be objects with their own type to override this field.
*/
itemType: ItemValueTypes;
/**
* 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};

displayUnit?: string;
granularity?: number;
}

export type TableItem = {
debugData?: DebugData;
[p: string]: undefined | string | number | boolean | undefined | DebugData | NodeValue | SourceLocationValue | LinkValue | UrlValue | CodeValue;
[p: string]: undefined | Value | Value[];
}

export interface OpportunityColumnHeading {
Expand All @@ -118,6 +124,11 @@ declare global {
* could also be objects with their own type to override this field.
*/
valueType: ItemValueTypes;
/**
* 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};

// NOTE: not used by opportunity details, but used in the renderer until table/opportunity unification.
displayUnit?: string;
Expand All @@ -130,7 +141,7 @@ declare global {
totalBytes?: number;
wastedMs?: number;
debugData?: DebugData;
[p: string]: number | boolean | string | undefined | DebugData;
[p: string]: undefined | Value | Value[];
}

/**
Expand Down