-
Notifications
You must be signed in to change notification settings - Fork 29
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: avatar from github map #195
Conversation
@@ -9,6 +9,12 @@ import { taskManager } from "../home"; | |||
// Map to track ongoing avatar fetches | |||
const pendingFetches: Map<string, Promise<Blob | void>> = new Map(); | |||
|
|||
export async function fetchPartnerAvatars(): Promise<GitHubIssue[]> { | |||
const response = await fetch("https://raw.githubusercontent.com/ubiquity/devpool-directory/__STORAGE__/devpool-partner-avatars.json"); |
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 have mixed feelings on your approach. The normal operating procedure is to create a pull request and link it to this task. Editing a production system is highly risky and prone to breakage. In the future you should work off of your own forks for a proof of concept.
ubiquity/devpool-directory@da89c2b
Mixed feelings, though, because I could see myself doing the same thing given that we're the only users of this directory at the moment.
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.
mb, I only did that cause the change was independent from the rest of the system. so it was self contained. even if it failed it wouldnt compromise any other section, but I agree its dumb
Alright so in devpool-directory backend im fetching all orgs avatar_urls and storing in a map. Then once we load the page we also load the map. This way we dont even store images, its all URLs, we dont even need cache. https://github.com/ubiquity/devpool-directory/blob/__STORAGE__/devpool-partner-avatars.json |
You're missing an avatar [
{
"ownerName": "ubiquity",
"avatar_url": "https://avatars.githubusercontent.com/u/76412717?v=4"
},
{
"ownerName": "ubiquity-os-marketplace",
"avatar_url": "https://avatars.githubusercontent.com/u/182627615?v=4"
},
{
"ownerName": "0x4007"
},
{
"ownerName": "ubiquity-os",
"avatar_url": "https://avatars.githubusercontent.com/u/160213852?v=4"
}
] |
fixed |
In the future I suggest adding a link in the commit message. For example:
Then I can conveniently see the linkback in the timeline here and view the changes. These details help a ton with auditing the work later on. |
const pendingFetches: Map<string, Promise<Blob | void>> = new Map(); | ||
export type OrgNameAndAvatarUrl = { | ||
ownerName: string; | ||
avatar_url?: string; |
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 feel like its better practice to not make property name optional, but instead to initialize it as null
if it is being set later in the program.
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.
good catch it doesnt even need to be nullable
Any luck with cypress? I would try on a new branch to fix because I always have problems with it |
Resolves #194
You can QA this by opening console, indexedDb, GitHubAvatars, imageStore, then check the timestamps. Once 15 minutes have passed it should re-cache so timestamp should change.