-
Notifications
You must be signed in to change notification settings - Fork 105
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
Introduce docs on adding new facades #119
Conversation
@addyosmani .. me patrick and adam revised this a bit and i brought it over here. holla if you have suggestions. |
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.
meat of this looks fantastic 👌
facades.md
Outdated
@@ -0,0 +1,58 @@ | |||
# Adding a new facade to third-party-web | |||
|
|||
Lighthouse 7.0 includes a new audit to flag third-party |
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.
this paragraph feels like it might belong in lighthouse or web.dev pointing to this doc?
here it feels like just a sentence saying this powers lighthouse would work
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.
sure. i tweaked it.
since we're spread across LH report, web.dev docs, this doc, etc.. I still want a little bit of context everywhere. it's redundant but whatever.
Co-authored-by: Patrick Hulce <[email protected]>
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.
Overall LGTM, thanks for for the help refining this!
facades.md
Outdated
@@ -0,0 +1,58 @@ | |||
# Facade data in third-party-web | |||
|
|||
In addition to identifying entities and products, third-party-web also includes data on available alternative libraries for products known as _facades_. A _facade_ is a static element which looks similar to the actual embedded resource, but is not functional and therefore much less taxing on the page load. |
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.
Defining "facade" here seems redundant since we have the terminology section below.
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.
agree. @patrickhulce cool if we drop it here? or we just inline the terminology into sentences up here.
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.
inline up here SGTM. I wanted an intro to the purpose of this document for the rest of the community and contributors that wasn't "Lighthouse has a new audit" :)
terminology section was helpful for broad context google doc, but can just be in intro here SGTM
Co-authored-by: Patrick Hulce <[email protected]>
Co-authored-by: Adam Raine <[email protected]>
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.
thanks a lot for working this up @adamraine and @paulirish !
facades.md
Outdated
## Criteria for adding a new facade | ||
### Basic functionality | ||
|
||
* Loads an HTML component on page load which looks like the actual third-party |
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.
Imo, a facade should present an approximate of the actual third-party, but it doesn't strictly need to load an HTML component to do this. It's valid if you have an image/screenshot/generated scrape of the 3P that loads the real thing on interaction.
Something like https://twitter.com/wongmjane/status/1330273157245243394 is an example of where an image can be fine here.
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.
Isn't your example an HTML component?
I interpreted "HTML component" here to mean anything that shows up in the DOM as compared to a library/utility that only exists in JS.
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.
yeah i was going with a loose definition of "component". There doesn't feel like there's a great word for this thing. It's not a "library".... even "widget" is close but doesn't feel right.
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.
Patrick: after taking a second look, you're of course correct. I know finding the right wording here is tricky and so HTML component may be totally fine if its to be interpreted with a loose definition.
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.
If it took this conversation to explain thats what we're talking about to someone already very familiar with the area though, perhaps thats enough of a signal that others might not understand what we're talking about :)
Is there an alternative term you had in mind @addyosmani that has a better blend of the generic yet specific we need here?
|
||
### Well Maintained | ||
|
||
* The projects issues and contributions are managed responsibly. Bugs are |
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.
Minor: Is there any concern we aren't defining what we mean by swiftly
here? e.g perhaps we could provide an example (swiftly (e.g within 30-60 days of filing)
).
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 think we were intentionally vague on these points, @paulirish and I had quite the scuffle as to whether it's better to have these discussions on a case-by-case basis or a settled policy ;)
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.
yeah i just thinking we may be adding more governance than necessary. but swiftly (e.g within 30-60 days of filing)
wfm.
|
||
* The projects issues and contributions are managed responsibly. Bugs are | ||
handled swiftly. | ||
* Is already used in production, ideally by users other than the creator. |
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.
Would this requirement disqualify some of the options we were looking to recommend earlier? e.g Alex's vimeo lite component. I'm not saying this shouldn't be a recommendation (rather, I think production use is a great requirement), just pointing this out... :)
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 think it would disqualify many, but I think that's why they're guidelines w/case-by-case determination instead of strict requirements and the intention was just to not broadcast that.
|
||
Read | ||
[contributing](https://github.com/patrickhulce/third-party-web#contributing) for | ||
basics; look in `data/entities.js` for `products` to see how the data is structured. |
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.
Rest of this looks great 👍
Co-authored-by: Patrick Hulce <[email protected]>
Hasn't been movement here in a bit. Any objections to merging? |
thanks for the reminder @adamraine, done :) |
to clear on authorship: @adamraine wrote most of this. I revised it and yote it over here.
ref GoogleChrome/lighthouse#11290