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

Frontend api cleaned #13

Merged
merged 7 commits into from
Aug 22, 2023
Merged

Frontend api cleaned #13

merged 7 commits into from
Aug 22, 2023

Conversation

navidboloorian
Copy link
Contributor

Changes

What changes did you make? Include screenshots if applicable, or explain how to view the changes.

  • Created fetch calls to createTask and the function outline for getAllTasks
  • "frontend-api" branch with cleaned commit history

Testing

How did you confirm your changes work? (Automated tests, manual verification, etc.)

  • Made calls from the frontend and verified that new tasks were being created in the database

Tracking

Add your issue number below.

Resolves #10

Copy link
Member

@wllmwu wllmwu left a comment

Choose a reason for hiding this comment

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

Looking good, just some suggestions about the comments. Unfortunately it's hard to tell line lengths in this GitHub suggestions interface so these comments might be less than 80 characters wide.

Overall I would try to be more consistently user-centered in writing doc comments, more explicit about all and only the things that the users of these functions should know about. I'd also try to be more precise with language/terminology -- for example, "the fetch call" was mentioned a lot, but to me that refers to the invocation of the fetch() JavaScript function, not the actual HTTP request being sent over the network.

frontend/src/api/requests.ts Outdated Show resolved Hide resolved
frontend/src/api/requests.ts Outdated Show resolved Hide resolved
frontend/src/api/requests.ts Outdated Show resolved Hide resolved
frontend/src/api/requests.ts Outdated Show resolved Hide resolved
frontend/src/api/requests.ts Outdated Show resolved Hide resolved
frontend/src/api/task.ts Outdated Show resolved Hide resolved
frontend/src/api/task.ts Outdated Show resolved Hide resolved
frontend/src/api/task.ts Outdated Show resolved Hide resolved
frontend/src/api/task.ts Outdated Show resolved Hide resolved
frontend/src/api/task.ts Outdated Show resolved Hide resolved
Copy link
Member

@wllmwu wllmwu left a comment

Choose a reason for hiding this comment

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

Let's merge 💪

Also I think you missed 6 comments that GitHub hid automatically:
Screenshot 2023-08-21 at 19 56 03

I went ahead and applied the suggestions. Not a fan of GH's UI these days, it's easy to miss

@wllmwu wllmwu merged commit 7184f38 into main Aug 22, 2023
2 checks passed
@wllmwu wllmwu deleted the frontend-api-cleaned branch August 22, 2023 02:59
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.

Write frontend API client
2 participants