-
-
Notifications
You must be signed in to change notification settings - Fork 42
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
[WIP] Add core web vitals report type #728
Conversation
…ding it into the main.html
{ | ||
"results": [ | ||
{ | ||
"technology": "wordpress", | ||
"dates": [ | ||
{ | ||
"date": "2022-04-22T00:00:00.000Z", | ||
"tested": 10, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tunetheweb using mock responses for the data for now. This is the structure we discussed a while back, but would be great with a sample output with realistic data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going to try to play with an alternative approach this week and next. Will let you know how it goes.
"good_overall": { | ||
"desktop": 4, | ||
"mobile": 7, | ||
"dataset": 6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tunetheweb I guess an alternative to including the "across all technologies with the same filter applied" value like this is always returning an object for "technology": "all"
along with the selected technologies.
eg:
{
"result": [
{
"technology": "squarespace",
"dates": [ ... ],
},
{
"technology": "wordpress",
"dates": [ ... ],
},
{
"technology": "all",
"dates": [ ... ],
}
]
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not started playing with it, but I think we should have one API call per technology. And include dates, mobile, desktop, Core Web Vitals, Lighthouse...etc. in it.
all
will also count as a technology too.
config/techreport.json
Outdated
"pages": { | ||
"landing": { | ||
"id": "landing", | ||
"title": "Technology Report", | ||
"subtitle": "Report", | ||
"description": "This is placeholder text about how the report works", | ||
"sections": [ | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tunetheweb something to discuss: to which extend do we want to configure the pages / sections / components in a json file vs. building the page structure directly in the techreport templates?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm OK with NOT putting too much into JSON. It works well for the other reports where they are all basically copies of each other with different config. But this will be it's own, individual one so quite different.
Then again, having the labels in JSON would allow us to potentially offer the report in alternative languages. But should be easy enough to convert the hardcoded labels to config later.
config/mock_responses/adoption.json
Outdated
{ | ||
"results": [ | ||
{ | ||
"technology_name": "Wordpress", | ||
"origins": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tunetheweb is it correct that we'll have one api per metric? or will adoption, metadata, CWVs, lighthouse audits, etc. all come as a result from the same call?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per above, I've not started playing with it, but I think we should have one API call per technology. And include dates, mobile, desktop, Core Web Vitals, Lighthouse...etc. in it.
all
will also count as a technology too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I'm configuring some of the components now, it feels like it would indeed be easier to have one API call per tech and then include everything in there.
Would also be useful if the results will be formatted more or less the same way, so we can do something reusable along the lines of tech.results[metric][selected_client]
(with metric being eg. origins, good_cwvs, lighthouse_performance, etc) to render the values.
config/mock_responses/adoption.json
Outdated
] | ||
} | ||
] | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, will fix
…into cwvtech-structure
Note:
The PR is focused on:
Included:
techreport.json
config filedocs/cwv_tech_report.md
To be added
PR can be reviewed even though these are not added yet:
In follow-up PR(s) this autumn:
Later:
What we know of already: