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

Fixes #66 added user/progress mock endpoint #117

Closed

Conversation

ayushjain94
Copy link
Collaborator

GET request has been added for user/progress. Have created a new directory which we can remove later once the endpoint is ready or this may help while writing the test cases.

@vegetabill vegetabill self-assigned this Oct 1, 2020
@vegetabill vegetabill self-requested a review October 1, 2020 22:57
routes/routes.js Outdated Show resolved Hide resolved
@vegetabill vegetabill linked an issue Oct 1, 2020 that may be closed by this pull request
Copy link
Collaborator

@vegetabill vegetabill left a comment

Choose a reason for hiding this comment

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

Also, this might've gotten lost in the bottom of the issue description but we also want to call this endpoint from the browser:

Once the main.js loads, add an AJAX call (using fetch, not jQuery) to the above endpoint and set the current question to the returned currentQuestionNumber

public/scripts/main.js Outdated Show resolved Hide resolved
public/scripts/main.js Outdated Show resolved Hide resolved
function getUserProgress() {
return fetch(document.URL + 'user/progress')
}

function nextQuestion() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did auto formatting from this line to the last...Let me know if I need to remove this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes actually can you please only include changes relevant to the PR. Unfortunately we didn't get #89 merged prior to OSD which would've helped. Otherwise we'll get a bunch of spurious conflicts later.

If you're using VSCode, they added a really nice feature to only auto-format modified lines which you can enable in settings. Search settings for "Format". That really helps with these messy and non-standardized codebases 😄

If you're using another editor, one technique I've used is to do git add -p when staging which lets me just choose the hunks I've edited and not the rest of them produced by any auto-formatting. HTH

Copy link
Collaborator

@vegetabill vegetabill left a comment

Choose a reason for hiding this comment

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

Your changes look great! I just had a few revision requests when you get a chance. Thanks for working on this.

function getUserProgress() {
return fetch(document.URL + 'user/progress')
}

function nextQuestion() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes actually can you please only include changes relevant to the PR. Unfortunately we didn't get #89 merged prior to OSD which would've helped. Otherwise we'll get a bunch of spurious conflicts later.

If you're using VSCode, they added a really nice feature to only auto-format modified lines which you can enable in settings. Search settings for "Format". That really helps with these messy and non-standardized codebases 😄

If you're using another editor, one technique I've used is to do git add -p when staging which lets me just choose the hunks I've edited and not the rest of them produced by any auto-formatting. HTH

if (!response.ok) {
const message = `An error has occured: ${response.status}`;
console.error(message)
return 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for adding error handling.

However, just by reading, doesn't the returned value have to be { currentQuestionNumber: 1 } for the next part to work? I haven't tried it so I could be wrong.

$('.container').css('height', $(window).height());
});
});

function getUserProgress() {
return fetch(document.URL + 'user/progress')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you change this to just a relative url of /user/progress? We don't really need the other part explicitly.

public/scripts/main.js Outdated Show resolved Hide resolved
@ayushjain94
Copy link
Collaborator Author

Hi, I am sorry how this got closed, Just reopen it again if there are any conflicts or needs revision.

@ayushjain94
Copy link
Collaborator Author

This PR hasn't been merged, so I'll just open a new PR and push only relevant code as discussed here. My apologies for the mess.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

First GET request: /user/progress
3 participants