-
Notifications
You must be signed in to change notification settings - Fork 8
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
feat: make api/roadmap call from client #181
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
noticed that filling out the form from landing page is currently broken. working on fixing it. |
fixed |
ugh. it's late and i'm tired but 1151 is working on the demo site now: cc @djmcquillan @whizzzkid |
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 non-blocking nits. Great work!
This opens doors for showing a loading skeleton and hopefully breakdown the API even further so that we can fetch child nodes async independently.
if (usePendingChildren) { | ||
pendingChildren = childrenParsed.map(({html_url}) => ({html_url, group: issueData.title, parentHtmlUrl: issueData.html_url})) | ||
} else { | ||
children = await resolveChildrenWithDepth(await resolveChildren(childrenParsed)) |
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: we can break this over multiple lines
const urlErrors: StarMapsIssueError[] = [] | ||
errorsForUrl.forEach((starMapError) => { | ||
urlErrors.push({ | ||
// errors: | ||
userGuideUrl: starMapError.userGuideUrl, | ||
title: starMapError.title, | ||
message: starMapError.message, | ||
}); | ||
|
||
}); |
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:
const urlErrors: StarMapsIssueError[] = [] | |
errorsForUrl.forEach((starMapError) => { | |
urlErrors.push({ | |
// errors: | |
userGuideUrl: starMapError.userGuideUrl, | |
title: starMapError.title, | |
message: starMapError.message, | |
}); | |
}); | |
const urlErrors: StarMapsIssueError[] = errorsForUrl.map(({ | |
userGuideUrl, title, message | |
}) => ({ | |
userGuideUrl, title, message | |
}); |
if (globalLoadingState.get()) return; | ||
const fetchRoadMapResponse = async () => { | ||
globalLoadingState.start(); | ||
const roadmapApiUrl = `${window.location.origin}/api/roadmap?owner=${owner}&repo=${repo}&issue_number=${issue_number}` |
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: we should map VERCEL_URL
from process.env
instead of relying on window.location.origin
https://vercel.com/docs/concepts/projects/environment-variables#system-environment-variables
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.
Unfortunately, that actually breaks for preview urls
* main: feat: make api/roadmap call from client (#181)
* fix: add landing page github linkout #150 * remove extra added code * github png logo * feat: make api/roadmap call from client (#181) * Codeclimate Coverage Reporter * fix * rename * Rename to CI * Adding badges * Updating * fix: coverage reporting for all files (#191) * removed github link from md and added to client * updated style of link back to orginal * chore: update starmapsGithubUrl Co-authored-by: Russell Dempsey <1173416+SgtPooki@users.noreply.github.com> Co-authored-by: Nishant Arora <1895906+whizzzkid@users.noreply.github.com>
Summary of this PR:
fix/bacalhau-1151-with-async-children
)commits
This issue continues along with a number of other issues to resolve #158:
Prior to this change
browser network size and timing
After this change (from localhost)
browser network size
Initial page load timing
Roadmap API timing