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

core(details-renderer): snippet details renderer type #6999

Merged
merged 20 commits into from
Feb 20, 2019

Conversation

mattzeunert
Copy link
Contributor

@mattzeunert mattzeunert commented Jan 13, 2019

Summary

Details renderer type that shows a code snippet with error highlights.

Example LH report using code snippets for structured data

screenshot 2019-01-13 at 15 54 45

Status

Not 100% finished but would be good to get some initial review.

Functionality is complete now.

Reverted JSON-LD commit is just here because I use it for testing.

Related Issues/PRs

Closes: #6901

@mattzeunert
Copy link
Contributor Author

Getting this on local, is there anything else I need to do for i18n?

google.protobuf.json_format.ParseError: Failed to parse i18n field: Failed to parse rendererFormattedStrings field: Message type "I18n.RendererFormattedStrings" has no field named "codeSnippetCollapse".

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

really neat and powerful little snippet renderer! insanely better UX for the code I want to steal this for the unused CSS now :D

lighthouse-core/report/html/templates.html Outdated Show resolved Hide resolved
lighthouse-core/report/html/renderer/util.js Outdated Show resolved Hide resolved
lighthouse-core/report/html/renderer/details-renderer.js Outdated Show resolved Hide resolved
lighthouse-core/test/audits/audit-test.js Outdated Show resolved Hide resolved
@patrickhulce
Copy link
Collaborator

Getting this on local, is there anything else I need to do for i18n?

yeah you'll have to update the I18n.RendererFormattedStrings message in proto/lighthouse-result.proto with the new property

@mattzeunert
Copy link
Contributor Author

We'd like the error message text to always wrap within the visible area, so you can read it without scrolling. Like this:

screenshot 2019-01-23 at 20 31 20

But right now it doesn't work if the lines themselves are scrollable:

screenshot 2019-01-23 at 20 31 28

@rviscomi and I are ok with it, since having error messages that long is pretty rare. But let me know if you know some magic CSS trick that would fix this 🙂

This is the live example.

@mattzeunert mattzeunert changed the title WIP: core(details-renderer): code-snippet details renderer type core(details-renderer): code-snippet details renderer type Jan 29, 2019
@mattzeunert
Copy link
Contributor Author

@patrickhulce What do you think of the PR now?

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

lookin' real good!!

for the amount of complexity that's here I'm hoping for a bit broader test coverage if possible. Especially around some of the cases I'm talking about in the code where I'm confused :)

lighthouse-core/audits/audit.js Outdated Show resolved Hide resolved

snippet.append(CodeSnippetRenderer.renderSnippetLines(dom, tmpl, details));

// If expanded view still doesn't include all lines then indicate that that
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/that that/that/

Copy link
Collaborator

Choose a reason for hiding this comment

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

also what about the collapsed view?

I'm very shaky on all the omitted lines placeholder logic, why wouldn't the other chunk of logic handle this case already?

if lines[0].number !== 1 then wouldn't line && !previousLine be true in renderSnippetLines for lines[0]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also what about the collapsed view?

We don't show it there, but maybe we should. The reason against showing it is that it makes the UI look more cluttered. Usually it's clear from the code that it's cut off and you have the "Expand snippet" button as well.

screenshot 2019-02-02 at 07 56 26

let hasSeenHighlight = false;
for (let lineNumber = 1; lineNumber <= lineCount; lineNumber++) {
const {line, previousLine} = getLineAndPreviousLine(lines, lineNumber);
const {line: collapsedLine, previousLine: collapsedPreviousLine} =
Copy link
Collaborator

Choose a reason for hiding this comment

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

WDYT about some comments or more descriptive names here?

is this right?

  • !previousLine is just telling us if this current line is the start of a visible block and we need to render the omitted lines placeholder for the stuff that came before?
  • collapsedLine is just telling us if this current line should be shown even when the snippet is collapsed?
  • !collapsedPreviousLine is just telling us if this current line is the start of a visible block and we need to render the omitted lines placeholder for the stuff that came before?

Copy link
Contributor Author

@mattzeunert mattzeunert Feb 3, 2019

Choose a reason for hiding this comment

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

I've cleaned up the rendering logic a bit more, though it's still not exactly nice code 🤔

Let me know what you think.

lighthouse-core/report/html/renderer/details-renderer.js Outdated Show resolved Hide resolved
lighthouse-core/test/audits/audit-test.js Show resolved Hide resolved
@mattzeunert mattzeunert force-pushed the code-snippet branch 2 times, most recently from 5bbd284 to c4237ed Compare February 2, 2019 12:00
@mattzeunert mattzeunert changed the title core(details-renderer): code-snippet details renderer type core(details-renderer): snippet-list details renderer type Feb 2, 2019
@mattzeunert
Copy link
Contributor Author

for the amount of complexity that's here I'm hoping for a bit broader test coverage if possible. Especially around some of the cases I'm talking about in the code where I'm confused :)

I think the cases where you're confused about line rendering all already have test cases 😂 Maybe you meant something else, or I need make the test names more descriptive?

@mattzeunert
Copy link
Contributor Author

FAIL ./dist/viewer/src/viewer.js: 65.11kB > maxSize 65kB gzip

So sad 😞

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

Looking good so far! I'm really liking this new renderer.

Also, very brave to go wading into the details types, sorry they're in such rough shape :S

lighthouse-core/report/html/renderer/util.js Outdated Show resolved Hide resolved
lighthouse-core/report/html/renderer/details-renderer.js Outdated Show resolved Hide resolved
};
}

class SnippetListRenderer {
Copy link
Member

Choose a reason for hiding this comment

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

I feel like we need an overview for this type/renderer somewhere, though I'm not sure exactly where. Basically the only way to understand how it works right now is to read this file :)

In this file? On the details type in audit.d.ts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added some comments in audit-details.d.ts.

types/audit.d.ts Outdated Show resolved Hide resolved
lighthouse-core/audits/audit.js Outdated Show resolved Hide resolved
@brendankenny
Copy link
Member

FAIL ./dist/viewer/src/viewer.js: 65.11kB > maxSize 65kB gzip

This might be unavoidable because right now the viewer is like 60K after gzip and it includes all report renderer code twice (once for rendering, once for self replication), so a 11K+ new file would be expected to run right into that. There may be no choice but to bump the limit.

@brendankenny
Copy link
Member

brendankenny commented Feb 8, 2019

having spent some time back in details land looking at basically all of our existing audits, I'd like to propose splitting this (back?) into two details types, list and snippet.

I've noticed that we already have several audits using single-column tables to get an ugly list, and switching them to a dedicated list type (sometime in the future) would be great, so let's leave that path open to them. So:

export interface DetailsRendererList {
  type: 'list',
  items: DetailsRendererSnippet[]
} 

aka items still has to be an array of only snippets, but that will allow us to easily expand what's allowed in items in the future. THen

  • move snippet-list-renderer.js to snippet-renderer.js, and move SnippetListRenderer.render() into details-renderer.js as DetailsRenderer._renderList()
  • SnippetListRenderer.renderSnippet() moves to SnippetRenderer.render(), and the new DetailsRenderer._renderList() would then call that (fine to leave _renderList() specialized as it is because we only support rendering snippets in a list for now.
  • add type: 'snippet' to the DetailsRendererSnippet interface

I don't think this will be much of a code change, but it will leave us with plenty of flexibility.

WDYT?

This reverts commit 249dd67b984c0071a29bcd9190092e98dc52f1cb.
@mattzeunert mattzeunert changed the title core(details-renderer): snippet-list details renderer type core(details-renderer): snippet details renderer type Feb 11, 2019
@mattzeunert
Copy link
Contributor Author

Have separated snippet and list into separate details renderer types.

Also, highlights is now lineMessages and generalMessages.

export interface List {
  type: "list";
  items: Snippet[];
}

export interface Snippet {
  type: "snippet";
  lines: {
    content: string;
    lineNumber: number;
    truncated?: boolean;
  }[];
  title: string;
  lineMessages: {
    lineNumber: number;
    message: string;
  }[];
  generalMessages: {
    message: string;
  }[];
  lineCount: number;
  node?: NodeValue;
}

@danbri
Copy link

danbri commented Feb 14, 2019

How does it get its knowledge of what terms and structures are in Schema.org? e.g. the "unexpected property" in the screenshot at the top of this PR. /cc @vholland @rvguha

@paulirish
Copy link
Member

paulirish commented Feb 14, 2019 via email

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

LGTM!

lighthouse-core/report/html/renderer/details-renderer.js Outdated Show resolved Hide resolved
lighthouse-core/report/html/renderer/snippet-renderer.js Outdated Show resolved Hide resolved
lighthouse-core/report/html/renderer/snippet-renderer.js Outdated Show resolved Hide resolved
// @ts-ignore - TODO(bckenny): Fix type hierarchy
/** @type {LH.Audit.Details.List} */ (details)
);
case 'snippet':
Copy link
Member

Choose a reason for hiding this comment

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

let's not add snippet here (it's just going to be immediately removed anyways)


details.items.forEach(item => {
// @ts-ignore TODO(bckenny): this can never be null
listContainer.appendChild(this.render(item));
Copy link
Member

Choose a reason for hiding this comment

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

rather than call this.render(item), just call SnippetRenderer.render() directly (then also don't have to deal with null)

type: string,
items: Array<DetailsJSON>
}} ListDetailsJSON
*/
Copy link
Member

Choose a reason for hiding this comment

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

I think this can be deleted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup!

* Snippet of text with line numbers and annotations.
*/
export interface Snippet {
type: "snippet",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
type: "snippet",
type: 'snippet',

/**
* Snippet of text with line numbers and annotations.
*/
export interface Snippet {
Copy link
Member

Choose a reason for hiding this comment

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

SnippetValue to keep in line with the others, but otherwise this looks great!

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

LGTM!

📜 🔼
📜✂️📜
📜 🔽

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants