Skip to content
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

Figure out how to "upload" resources to GitHub #11

Merged
merged 2 commits into from
Nov 17, 2022

Conversation

saminarp
Copy link
Collaborator

Fixes #7

I have done my implementation based on PRs created for #1 and #4

Please review and let me know if this is the right approach. Feel free to review and request for changes.

Copy link
Collaborator

@batunpc batunpc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@saminarp I have left some review regarding the unused code.

Other than that I think it looks good! 😀

github-upload.js Outdated
@@ -0,0 +1,27 @@
import { Octokit, App } from "https://cdn.skypack.dev/octokit?dts";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

App does not need to be imported

github-upload.js Outdated
})
}

window.githubUpload = githubUpload
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line seems redundant.

Copy link
Collaborator

@CDPhu CDPhu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly, this is all I have to say, otherwise, it looks good!

github-upload.js Outdated
@@ -0,0 +1,27 @@
import { Octokit, App } from "https://cdn.skypack.dev/octokit?dts";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it not be better practice to import Octokit via node_modules? I get that this works, but apparently importing from HTTP is still experimental.
https://nodejs.org/api/esm.html#https-and-http-imports

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since my file was pretty small, I did it this way. I am not sure if there are any restrictions.

@SerpentBytes
Copy link
Collaborator

@saminarp Given that you coded the functions elegantly, I did not want to point out that App was imported but not used because it was too minor. Good work.

@saminarp
Copy link
Collaborator Author

I have updated my implementation, let me know how it is

Copy link
Collaborator

@batunpc batunpc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @saminarp, it looks good

Copy link
Collaborator

@CDPhu CDPhu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works

Copy link
Collaborator

@SerpentBytes SerpentBytes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@saminarp saminarp merged commit ba070eb into Seneca-CDOT:main Nov 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Figure out how to "upload" resources to GitHub
4 participants