feat: page speed fetch request #9
Conversation
|
📦 Extension packages built successfully! |
jonoliver
left a comment
jonoliver
left a comment
There was a problem hiding this comment.
Thanks Josh, works as expected! Just a few suggestions.
src/core/PageSpeedService.js
Outdated
| */ | ||
| export const makePageSpeedAPIRequest = async (urlToMeasure, logDebug = false) => { | ||
| // Proxy URL using serverless function that will return data. | ||
| const proxyRequestUrl = `https://carbon-calculator-proxy.netlify.app/.netlify/functions/page-speed?url=${encodeURIComponent(urlToMeasure)}`; |
There was a problem hiding this comment.
I think we should extract the API url to make it more easily configurable. It might ultimately make sense to use an ENV variable that gets injected to a hard-coded variable at build time, so that we could toggle between testing environments for the API. But for now, I'd say let's at least extract it to a PAGE_SPEED_API_URL const at the top of this file:
const PAGE_SPEED_API_URL = "https://carbon-calculator-proxy.netlify.app/.netlify/functions/page-speed";
src/panels/welcome/welcome.js
Outdated
| await extensionManager.openPanel('results'); | ||
| // For now, just show a simple message within the button. | ||
| analyzeBtn.disabled = true; | ||
| analyzeBtn.textContent = 'Test: Making request and logging to console...'; |
There was a problem hiding this comment.
I know this is temporary, but we should probably make the messaging a little more finalized for the user. How about: "Analyzing page..."
src/panels/welcome/welcome.js
Outdated
| } | ||
|
|
||
| // Reset button back to usable state. | ||
| analyzeBtn.textContent = 'Analyze This Page'; |
There was a problem hiding this comment.
This text is different from the original button text, which is "Analyze this website". I think your text actually makes more sense, since we're analyzing a specific page. I vote that we:
- Update the original text in
welcome.htmlto "Analyze this page" - Instead of repeating the text string here, capture the original message above, before it changes:
const analyzeBtnText = analyzeBtn.textContent; - Use the variable to reset the text here in place of the hard-coded string:
analyzeBtn.textContent = analyzeBtnText;
Basic page speed service function setup with debug logging. Update analyze button so it makes the the page speed fetch request to the testing URL. Updates async handling of the button and includes both console and a UI facing error message.
jawinn
force-pushed
the
feat--page-speed-implementation
branch
from
0d4caee to
b3b5b30
Compare
|
@jonoliver Thanks, I've updated with those suggestions, and rebased with main. |
Makes page speed request to the serverless function URL, and logs results to the console. Adds a PageSpeedService script. Includes both console and a UI facing error message on failure. Updates "Analyze" button so it makes the the page speed fetch request to the testing URL, and enables/disables the button with some testing in progress text.
Validation:
proxyRequestUrlto be a bad URLBLDL-9