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(flow): summary page #12973

Merged
merged 72 commits into from
Aug 26, 2021
Merged

report(flow): summary page #12973

merged 72 commits into from
Aug 26, 2021

Conversation

adamraine
Copy link
Member

Still a bit messy, and I haven't started the tests yet. Wanted to get some initial thoughts on the structure / looks before I finish up.

Gauge is an example of a component imported from the normal report renderer.

@google-cla google-cla bot added the cla: yes label Aug 23, 2021
flow-report/assets/styles.css Show resolved Hide resolved
flow-report/src/summary/summary.tsx Outdated Show resolved Hide resolved
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.

nice! great covering a lot of ground here

flow-report/assets/styles.css Outdated Show resolved Hide resolved
flow-report/assets/styles.css Outdated Show resolved Hide resolved
flow-report/assets/styles.css Outdated Show resolved Hide resolved
flow-report/assets/styles.css Outdated Show resolved Hide resolved
flow-report/assets/styles.css Outdated Show resolved Hide resolved
flow-report/src/common.tsx Outdated Show resolved Hide resolved
className="SummaryFlowStep__screenshot"
data-testid="SummaryFlowStep__screenshot"
src={screenshot || undefined}
style={{width: THUMBNAIL_WIDTH, maxHeight: thumbnailHeight}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

just set the height explicitly and rely on object-fit?

Copy link
Member Author

Choose a reason for hiding this comment

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

That cuts off part of the screenshot if it doesn't fit the aspect ratio of the emulated device screen. Navigation screenshots were this way before I switched to FPS.

Ultimately this is a bug with how the screenshots are collected, but I still think we should leave this as maxHeight in case they pop up again.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Gotcha, do we have an issue or entry in #11313 to tackle the screenshot bug you mention?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done #12988

flow-report/src/util.ts Show resolved Hide resolved
lighthouse-core/scripts/update-flow-fixtures.js Outdated Show resolved Hide resolved
@@ -96,6 +96,7 @@
"@firebase/auth-types": "0.3.2",
"@firebase/util": "0.2.1",
"@rollup/plugin-node-resolve": "^13.0.4",
"@rollup/plugin-typescript": "^2.1.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

haha, sounds good, but let's work to find a better reason for the perf packet later ;)

@adamraine adamraine merged commit 94f5bdf into master Aug 26, 2021
@adamraine adamraine deleted the fr-flow-summary branch August 26, 2021 20:19
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.

5 participants