-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[New sample] Open Chrome API reference page (omnibox, alarms, messaging sample) #848
Conversation
…sions-samples into sw-omnibox-api
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 looks great - just a super quick first pass with a few nits.
@@ -0,0 +1,106 @@ | |||
const apiList = [ |
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.
suggestion: move to a separate JSON file
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 are already giving an example of how to fetch with the extension tips feature. I think, if SW supported Import assertions, I would be more open to moving this into a separate JSON file.
How about I move it into a separate JS file? 🧐😁
// Send tip to content script via messaging | ||
chrome.runtime.onMessage.addListener((message, sender, sendResponse) => { | ||
if (message.greeting === 'tip') { | ||
chrome.storage.local.get('tip').then(sendResponse); |
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.
Please use async await
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 used .then()
here b/c the return value of the onMessage
listener determines if Chrome expects sendResponse
to be called.
If we use async/await, this function will return a Promise, and Chrome will always expect sendResponse
to be called.
We could wrap the call to .get()
in an async IIFE, but it seems like overkill for this tutorial😊
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 didn't know that, have you got a link to where this is explained. Curious to learn how this works.
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.
Here's the section that explain this, but I think it can be explained better:
// Don't use an async function to handle messages
// b/c an async function always returns a Promise object
// a Promise object is always truthy
// regardless of the value that the Promise contains
chrome.runtime.onMessage.addListener((message, sender, sendResponse) => {
if (message.greeting === "hello") {
// start a call to get a value from storage
chrome.storage.local.get("name").then(({ name }) => {
// this runs after the onMessage handler returns
sendResponse(`my name is ${name}`)
})
// the onMessage handler should return truthy if sendResponse will be called
return true
}
// the onMessage handler should return falsy if sendResponse will not be called
return false
})
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 ! That's not intuitive.
One thing: maybe rename the message field from greeting
to type
as it better expresses what this field is used for.
@sebastianbenz Thanks for the review. I applied most of your suggestions. I wrote comments on the suggestions I had a few thoughts on. |
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! Left a few more comments, only nits.
functional-samples/tutorial.open-api-reference/sw-suggestions.js
Outdated
Show resolved
Hide resolved
/** | ||
* Returns a list of suggestions and a description for the default suggestion | ||
*/ | ||
export async function getApiSuggestions(input) { |
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.
nit: can you pick a name that better explains what input is?
functional-samples/tutorial.open-api-reference/sw-suggestions.js
Outdated
Show resolved
Hide resolved
// Send tip to content script via messaging | ||
chrome.runtime.onMessage.addListener((message, sender, sendResponse) => { | ||
if (message.greeting === 'tip') { | ||
chrome.storage.local.get('tip').then(sendResponse); |
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 didn't know that, have you got a link to where this is explained. Curious to learn how this works.
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 looks good, once https://github.com/GoogleChrome/chrome-extensions-samples/pull/848/files#r1144016971 is fixed.
@AmySteam, later today adding a 'latest samples' entry to 'What's new'. It would be cool to include this, but I don't know how much more work you have. |
User Story
As a web developer, I want to open extension API reference pages on Chrome, so that I can quickly learn how to use a specific extension API.
Bonus: A daily extension tip will be displayed as a popover after clicking the "tip" navigation list item.
(Currently, it requires Canary build and the "Experimental Web Platform features" flag enabled. The release date was moved to M114, so this approach may need to be abandoned if we want to launch before then.)
How to use it?
TODO: