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

fix: clean up table styling #1726

Merged
merged 3 commits into from
Feb 15, 2017
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
24 changes: 17 additions & 7 deletions lighthouse-core/formatters/partials/table.css
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,6 @@
.table_list tr:hover {
background-color: #fafafa;
}
.table_list em {
color: #999;
font-style: normal;
margin-left: 10px;
}
.table_list code, .table_list pre {
white-space: pre;
font-family: monospace;
Expand All @@ -43,11 +38,26 @@
.table_list em + code, .table_list em + pre {
margin-top: 10px;
}
.table_list td img {
.table_list .table-column {
text-align: right;
}
.table_list .table-column.table-column-pre, .table_list .table-column.table-column-code {
text-align: left;
}
.table_list .table-column.table-column-url {
text-align: left;
width: 250px;
}
.table-column-potential-savings em, .table-column-webp-savings em, .table-column-jpeg-savings em {
color: #999;
font-style: normal;
padding-left: 10px;
}
.table-column-preview img {
height: var(--image-preview);
width: var(--image-preview);
object-fit: contain;
}
.table_list .preview-image {
.table-column-preview {
width: calc(var(--image-preview) * 2);
}
4 changes: 2 additions & 2 deletions lighthouse-core/formatters/partials/table.html
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,15 @@
<thead>
<tr>
{{#each ../tableHeadings}}
<th {{#ifEq @key "preview"}}class="preview-image"{{/ifEq}}>{{this}}</th>
<th class="table-column table-column-{{ kebabCase @key }}">{{this}}</th>
{{/each}}
</tr>
</thead>
<tbody>
{{#each rows}}
<tr>
{{#each cols}}
<td>{{ sanitize this}}</td>
<td class="table-column table-column-{{ kebabCase (lookup ../../headingKeys @index) }}">{{ sanitize this}}</td>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

don't like this, but not sure it's worth restructuring the format of rows just for class names here

Copy link
Contributor

@ebidel ebidel Feb 15, 2017

Choose a reason for hiding this comment

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

Nice. It's useful to be able to target these. It's hard that we have a generic table, but specific data that might need to look different here/there.

{{/each}}
</tr>
{{/each}}
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/formatters/table.js
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ class Table extends Formatter {

headings = headingKeys.map(key => headings[key]);

return {headings, rows};
return {headings, rows, headingKeys};
}

static getHelpers() {
Expand Down
15 changes: 15 additions & 0 deletions lighthouse-core/report/handlebar-helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,21 @@ const handlebarHelpers = {
return arg;
},

// myFavoriteVar -> my-favorite-var
kebabCase: str => {
return (str || '')
// break up camelCase tokens
.split(/([A-Z]+[a-z0-9]*)/)
// replace all special characters and whitespace with hyphens
.map(part => part.toLowerCase().replace(/[^a-z0-9]+/gi, '-'))
// rejoin into a single string
.join('-')
// de-dupe hyphens
.replace(/-+/g, '-')
// remove leading or trailing hyphens
.replace(/(^-|-$)/g, '');
},

// eslint-disable-next-line no-unused-vars
sanitize: (str, opts) => {
// const isViewer = opts.data.root.reportContext === 'viewer';
Expand Down
5 changes: 3 additions & 2 deletions lighthouse-core/test/formatter/table-formatter-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -81,15 +81,16 @@ describe('TableFormatter', () => {
const output = template(extendedInfo).split('\n').join('');
assert.ok(output.match('<table class="table_list'), 'creates a table');
assert.ok(output.match('multicolumn'), 'adds multicolumn class for large tables');
assert.ok(output.match('class="preview-image"'), 'renders image preview');
assert.ok(output.match(/class=\"[^"]*table-column-preview/), 'adds column className');
assert.ok(output.match(/class=\"[^"]*table-column-line-col/), 'adds multi-word className');

const extendedInfoShort = {
tableHeadings: {url: 'URL', lineCol: 'Line/col'},
results: extendedInfo.results
};
const output2 = template(extendedInfoShort).split('\n').join('');
assert.ok(!output2.match('multicolumn"'), 'does not add multicolumn class for small tables');
assert.ok(!output2.match('class="preview-image'),
assert.ok(!output2.match('table-column-preview'),
'does not add preview-image class if table does not have images');
});

Expand Down
9 changes: 9 additions & 0 deletions lighthouse-core/test/report/handlebar-helpers-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,4 +33,13 @@ describe('Handlebar helpers', () => {
const text = handlebarHelpers.not(false);
assert.ok(text);
});

it('`kebabCase` works properly', () => {
assert.equal(handlebarHelpers.kebabCase(undefined), '');
assert.equal(handlebarHelpers.kebabCase('foo'), 'foo');
assert.equal(handlebarHelpers.kebabCase('fooBarBaz'), 'foo-bar-baz');
assert.equal(handlebarHelpers.kebabCase('a long Phrase'), 'a-long-phrase');
assert.equal(handlebarHelpers.kebabCase('myURL$'), 'my-url');
assert.equal(handlebarHelpers.kebabCase('the401k%_value'), 'the401k-value');
});
});