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: move renderer code to report/ #12690

Merged
merged 14 commits into from
Jun 25, 2021
Merged

report: move renderer code to report/ #12690

merged 14 commits into from
Jun 25, 2021

Conversation

connorjclark
Copy link
Collaborator

@connorjclark connorjclark commented Jun 24, 2021

ref #12623, setup for #12689

Moving the renderer code from lighthouse-core/report to report/, and simplifying the directory structure. Better separation (core vs report), and provides room for us to continue to grow the report renderer (upcoming refactor + FR changes + maybe LHCI diff view?).

Renamed report-template.html to standalone-template.html, as I always confused it with templates.html. Also matches the standalone report terminology in the upcoming report renderer refactor.

Need to follow with a strings import. This PR includes a strings import.

report
├── README.md
├── assets
│   ├── standalone-template.html
│   ├── styles.css
│   └── templates.html
├── renderer
│   ├── category-renderer.js
│   ├── crc-details-renderer.js
│   ├── details-renderer.js
│   ├── dom.js
│   ├── element-screenshot-renderer.js
│   ├── i18n.js
│   ├── logger.js
│   ├── performance-category-renderer.js
│   ├── psi.js
│   ├── pwa-category-renderer.js
│   ├── report-renderer.js
│   ├── report-ui-features.js
│   ├── snippet-renderer.js
│   ├── text-encoding.js
│   └── util.js
├── report-assets.js
├── report-generator.js
└── test
    ├── renderer
    │   ├── category-renderer-test.js
    │   ├── crc-details-renderer-test.js
    │   ├── details-renderer-test.js
    │   ├── dom-test.js
    │   ├── element-screenshot-renderer-test.js
    │   ├── i18n-test.js
    │   ├── performance-category-renderer-test.js
    │   ├── psi-test.js
    │   ├── pwa-category-renderer-test.js
    │   ├── report-renderer-test.js
    │   ├── report-ui-features-test.js
    │   ├── snippet-renderer-test.js
    │   ├── text-encoding-test.js
    │   └── util-test.js
    └── report-generator-test.js

4 directories, 36 files

@connorjclark connorjclark requested a review from a team as a code owner June 24, 2021 06:36
@connorjclark connorjclark requested review from patrickhulce and removed request for a team June 24, 2021 06:36
@google-cla google-cla bot added the cla: yes label Jun 24, 2021

const TEMPLATE_FILE = fs.readFileSync(__dirname +
'/../../../../report/html/templates.html', 'utf8');
const reportAssets = require('../../report-assets.js');
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

minor tweak I did in these test files was dropping fs for report-assets.js

tsconfig.json Outdated Show resolved Hide resolved
@@ -640,7 +641,7 @@ if (require.main === module) {

if (collisions > 0) {
console.log(`MEANING COLLISION: ${collisions} string(s) have the same content.`);
assert.equal(collisions, 30, `The number of duplicate strings have changed, update this assertion if that is expected, or reword strings. Collisions: ${collisionStrings.join('\n')}`);
assert.equal(collisions, 28, `The number of duplicate strings have changed, update this assertion if that is expected, or reword strings. Collisions: ${collisionStrings.join('\n')}`);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

idk why this changed

Copy link
Collaborator

Choose a reason for hiding this comment

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

that's somewhat concerning... :/

can we do a diff?

this is what I see on master

    "When an element doesn't have an accessible name, screen readers announce it with a generic name, making it unusable for users who rely on screen readers. $LINK_START_0$Learn more$LINK_END_0$.",
    "When an element doesn't have an accessible name, screen readers announce it with a generic name, making it unusable for users who rely on screen readers. $LINK_START_0$Learn more$LINK_END_0$.",
    'ARIA $MARKDOWN_SNIPPET_0$ elements have accessible names',
    'ARIA $MARKDOWN_SNIPPET_0$ elements have accessible names',
    'ARIA $MARKDOWN_SNIPPET_0$ elements do not have accessible names.',
    'ARIA $MARKDOWN_SNIPPET_0$ elements do not have accessible names.',
    "When an element doesn't have an accessible name, screen readers announce it with a generic name, making it unusable for users who rely on screen readers. $LINK_START_0$Learn more$LINK_END_0$.",
    'ARIA $MARKDOWN_SNIPPET_0$ elements have accessible names',
    'ARIA $MARKDOWN_SNIPPET_0$ elements do not have accessible names.',
    "When an element doesn't have an accessible name, screen readers announce it with a generic name, making it unusable for users who rely on screen readers. $LINK_START_0$Learn more$LINK_END_0$.",
    'ARIA $MARKDOWN_SNIPPET_0$ elements have accessible names',
    'ARIA $MARKDOWN_SNIPPET_0$ elements do not have accessible names.',
    "When an element doesn't have an accessible name, screen readers announce it with a generic name, making it unusable for users who rely on screen readers. $LINK_START_0$Learn more$LINK_END_0$.",
    '$MARKDOWN_SNIPPET_0$ elements have $MARKDOWN_SNIPPET_1$ text',
    '$MARKDOWN_SNIPPET_0$ elements have $MARKDOWN_SNIPPET_1$ text',
    '$MARKDOWN_SNIPPET_0$ elements do not have $MARKDOWN_SNIPPET_1$ text',
    '$MARKDOWN_SNIPPET_0$ elements do not have $MARKDOWN_SNIPPET_1$ text',
    'Document has a valid $MARKDOWN_SNIPPET_0$',
    'Document has a valid $MARKDOWN_SNIPPET_0$',
    'Potential Savings',
    'Potential Savings',
    'Failing Elements',
    'Failing Elements',
    'URL',
    'URL',
    'Consider uploading your GIF to a service which will make it available to embed as an HTML5 video.',
    'Consider uploading your GIF to a service which will make it available to embed as an HTML5 video.',
    'Consider using a $LINK_START_0$plugin$LINK_END_0$ or service that will automatically convert your uploaded images to the optimal formats.',
    'Consider using a $LINK_START_0$plugin$LINK_END_0$ or service that will automatically convert your uploaded images to the optimal formats.',
    'Consider uploading your GIF to a service which will make it available to embed as an HTML5 video.'

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

"When an element doesn't have an accessible name, screen readers announce it with a generic name, making it unusable for users who rely on screen readers. $LINK_START_0$Learn more$LINK_END_0$.",
'ARIA $MARKDOWN_SNIPPET_0$ elements have accessible names',
'ARIA $MARKDOWN_SNIPPET_0$ elements do not have accessible names.',
"When an element doesn't have an accessible name, screen readers announce it with a generic name, making it unusable for users who rely on screen readers. $LINK_START_0$Learn more$LINK_END_0$.",
'ARIA $MARKDOWN_SNIPPET_0$ elements have accessible names',
'ARIA $MARKDOWN_SNIPPET_0$ elements do not have accessible names.',
"When an element doesn't have an accessible name, screen readers announce it with a generic name, making it unusable for users who rely on screen readers. $LINK_START_0$Learn more$LINK_END_0$.",
'ARIA $MARKDOWN_SNIPPET_0$ elements have accessible names',
'ARIA $MARKDOWN_SNIPPET_0$ elements do not have accessible names.',
"When an element doesn't have an accessible name, screen readers announce it with a generic name, making it unusable for users who rely on screen readers. $LINK_START_0$Learn more$LINK_END_0$.",
'$MARKDOWN_SNIPPET_0$ elements have $MARKDOWN_SNIPPET_1$ text',
'$MARKDOWN_SNIPPET_0$ elements do not have $MARKDOWN_SNIPPET_1$ text',
'Document has a valid $MARKDOWN_SNIPPET_0$',
'Potential Savings',
'Failing Elements',
'Consider uploading your GIF to a service which will make it available to embed as an HTML5 video.',
'Consider using a $LINK_START_0$plugin$LINK_END_0$ or service that will automatically convert your uploaded images to the optimal formats.',
'Consider uploading your GIF to a service which will make it available to embed as an HTML5 video.'

diff https://www.diffchecker.com/TFvWBE9j

Copy link
Collaborator

Choose a reason for hiding this comment

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

hm, that's 10 more missing than there should be, I had to add collisionStrings.push where the first collisions++ is

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh.. then:

image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

collectAllStringsInDir does have some local data per directory, and renderer strings moved to a new folder, so that must be why. no problem here i guess ... except maybe in the "collision detection" code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

@@ -1,30 +1,26 @@
# Lighthouse HTML Report Renderer
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

mostly just updated links here.

@connorjclark connorjclark mentioned this pull request Jun 24, 2021
17 tasks
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.

thanks for splitting this up, I like the new directory structure 👍

tsconfig.json Outdated Show resolved Hide resolved
@@ -640,7 +641,7 @@ if (require.main === module) {

if (collisions > 0) {
console.log(`MEANING COLLISION: ${collisions} string(s) have the same content.`);
assert.equal(collisions, 30, `The number of duplicate strings have changed, update this assertion if that is expected, or reword strings. Collisions: ${collisionStrings.join('\n')}`);
assert.equal(collisions, 28, `The number of duplicate strings have changed, update this assertion if that is expected, or reword strings. Collisions: ${collisionStrings.join('\n')}`);
Copy link
Collaborator

Choose a reason for hiding this comment

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

that's somewhat concerning... :/

can we do a diff?

this is what I see on master

    "When an element doesn't have an accessible name, screen readers announce it with a generic name, making it unusable for users who rely on screen readers. $LINK_START_0$Learn more$LINK_END_0$.",
    "When an element doesn't have an accessible name, screen readers announce it with a generic name, making it unusable for users who rely on screen readers. $LINK_START_0$Learn more$LINK_END_0$.",
    'ARIA $MARKDOWN_SNIPPET_0$ elements have accessible names',
    'ARIA $MARKDOWN_SNIPPET_0$ elements have accessible names',
    'ARIA $MARKDOWN_SNIPPET_0$ elements do not have accessible names.',
    'ARIA $MARKDOWN_SNIPPET_0$ elements do not have accessible names.',
    "When an element doesn't have an accessible name, screen readers announce it with a generic name, making it unusable for users who rely on screen readers. $LINK_START_0$Learn more$LINK_END_0$.",
    'ARIA $MARKDOWN_SNIPPET_0$ elements have accessible names',
    'ARIA $MARKDOWN_SNIPPET_0$ elements do not have accessible names.',
    "When an element doesn't have an accessible name, screen readers announce it with a generic name, making it unusable for users who rely on screen readers. $LINK_START_0$Learn more$LINK_END_0$.",
    'ARIA $MARKDOWN_SNIPPET_0$ elements have accessible names',
    'ARIA $MARKDOWN_SNIPPET_0$ elements do not have accessible names.',
    "When an element doesn't have an accessible name, screen readers announce it with a generic name, making it unusable for users who rely on screen readers. $LINK_START_0$Learn more$LINK_END_0$.",
    '$MARKDOWN_SNIPPET_0$ elements have $MARKDOWN_SNIPPET_1$ text',
    '$MARKDOWN_SNIPPET_0$ elements have $MARKDOWN_SNIPPET_1$ text',
    '$MARKDOWN_SNIPPET_0$ elements do not have $MARKDOWN_SNIPPET_1$ text',
    '$MARKDOWN_SNIPPET_0$ elements do not have $MARKDOWN_SNIPPET_1$ text',
    'Document has a valid $MARKDOWN_SNIPPET_0$',
    'Document has a valid $MARKDOWN_SNIPPET_0$',
    'Potential Savings',
    'Potential Savings',
    'Failing Elements',
    'Failing Elements',
    'URL',
    'URL',
    'Consider uploading your GIF to a service which will make it available to embed as an HTML5 video.',
    'Consider uploading your GIF to a service which will make it available to embed as an HTML5 video.',
    'Consider using a $LINK_START_0$plugin$LINK_END_0$ or service that will automatically convert your uploaded images to the optimal formats.',
    'Consider using a $LINK_START_0$plugin$LINK_END_0$ or service that will automatically convert your uploaded images to the optimal formats.',
    'Consider uploading your GIF to a service which will make it available to embed as an HTML5 video.'

lighthouse-report/test/proto-test.js Outdated Show resolved Hide resolved
@connorjclark
Copy link
Collaborator Author

can we do a strings import w/o actually committing? I think so. there's a test that is failing b/c of missing renderer strings in en-XA

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.

are we seriously going to do another lighthouse-*/ directory? :P

report/!

@connorjclark
Copy link
Collaborator Author

Haha I agree, but was gonna suggest renaming all of those in another PR and did lighthouse-report just to match. Would lighthouse-core to core be breaking?

Could also just make it report now. Wdyt

@brendankenny
Copy link
Member

ha, I wasn't sure if anyone else would agree :) It probably makes sense to start with just report/ and do it here, but not sure if anyone else cares

@connorjclark
Copy link
Collaborator Author

ok i'm doing it
image

@connorjclark connorjclark changed the title report: move renderer code to lighthouse-report report: move renderer code to report/ Jun 25, 2021
@connorjclark
Copy link
Collaborator Author

imported strings too

@connorjclark
Copy link
Collaborator Author

@paulirish can you turn off CDT tests for a bit? at least until the renderer changes land sometime next week. this PR renames template.html to standalone-template.html, which means some BUILD crap in CDT needs to change too, but we can't do it all at once. and then, many things need to change in BUILD for es modules to land. so best to disable for a bit.

@connorjclark
Copy link
Collaborator Author

@patrickhulce so we agree the string collision is not a real problem, and we will hash it out eventually (but no reason to block this PR). anything else?

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

Successfully merging this pull request may close these issues.

4 participants