-
-
Notifications
You must be signed in to change notification settings - Fork 11
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
[WIP] Add a GUI #59
base: main
Are you sure you want to change the base?
[WIP] Add a GUI #59
Conversation
Hey @CapriciousRebel! Nice work all around. Really excited for the GUI! Just thought I'll add a few thoughts here:
|
@thehanimo thanks for your comments! |
3454928
to
fc3d95a
Compare
@rosekamallove I have squashed all the commits into one, and fixed all eslint issues as well, now you can rebase and continue work! |
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.
@CapriciousRebel I still see src/static/style.css
with 188,032 lines?
It is the 4th task, see the first comment |
Got it! My bad. |
@rosekamallove added the purge command, kindly rebase once. |
@rosekamallove what is the update on your tasks? |
Will push the changes by the end of this week, Diwali |
const { getMetrics } = require('../metrics'); | ||
|
||
// Metric names and their extraction method to be used on the query result (Order is preserved) | ||
const metricNames = [ |
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.
seems like this file contains a lot of copied code from elsewhere in the repo?
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.
Yes, it is the code from the details.js
file. Kindly consider this commit as an intermediate step. I was having some trouble with the exports(for some reason importing the detail
function gave errors, saying detail wasn't a function). So I just copy-pasted the code instead to make things work for the time being. I will fix this in a future commit and perhaps squash this commit.
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.
Sounds good :-) just calling it out so it's not forgotten.
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 is still a doubt for me, could you probably suggest me a fix for this? I'm not sure why importing from detail.js is not working. Perhaps there is some issue with express js? Not really sure
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'm not sure either. Can you restore the import, and i'll try it locally?
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.
Let's address this issue at the very end of this PR, once we have everything in working order.
@ljharb a gentle reminder for a review on this PR |
Will update the PR before the end of the year :) |
@ljharb updated my PR, left replies on your comments. Could you please give a further review? |
@ljharb updated my PR, also left replies on some comments for further discussion. |
@ljharb I have left more replies on our conversations, I guess we are really close to finishing this PR :) |
also see some of the hidden conversations (#59 (comment)) |
@ljharb updated my PR to include the token as a flag or positional argument from the frontend itself, also added a minimal error handler as well for when the user doesn't provide the token, maybe someday we could evolve the error handling to a modal in the frontend, after having handled all errors in the backend (ex: Bad Credentials error for GitHub token is not handled yet in the backend) Also could you please address #59 (comment) |
I'm confused; the frontend shouldn't be supplying the token - the invocation to start the server should be. |
I see, so basically, a person does this:
This flow makes perfect sense to me, but I am a bit confused as to where will the token be stored during this time?
Summary: Where would the GitHub token be stored once the user provides it at the time of GUI invocation so that the user doesn't have to provide it every time from the frontend via the command? |
@ljharb gentle reminder to give an update on this PR |
Currently, the GUI is a work in progress.
How to use it?
npm install
..env
file.--gui
optionlocalhost:3000
!To run a different command, go back to terminal, and run another command along with the
--gui
option (close the terminal that was running first withCmd + C
), then come back and refresh the browserAt this stage, I have used the following stack:
Ejs: Generating HTML using templating.
Express: Serving the generated HTML and the static files(CSS styles).
TailwindCSS: For styling the HTML.
Tasks: