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

[Refactor]: Opportunity Api calling methods #71

Open
wants to merge 2 commits into
base: refactor/api_call
Choose a base branch
from

Conversation

Tanvir-rahman
Copy link
Collaborator

No description provided.

@Tanvir-rahman Tanvir-rahman added the refactor refactor previous code label Jun 14, 2022
@Tanvir-rahman Tanvir-rahman requested a review from momaqbol June 14, 2022 19:02
@Tanvir-rahman Tanvir-rahman self-assigned this Jun 14, 2022
@@ -102,7 +102,12 @@ const Projects = {
}

const Opportunities = {
list: () => requests.get<Opportunity[]>('opportunities'),
list: (params?: URLSearchParams) => requests.get<Opportunity[]>('opportunities', params),
post: (body: Opportunity) => requests.post<Opportunity>('opportunities', body),
Copy link
Collaborator Author

@Tanvir-rahman Tanvir-rahman Jun 14, 2022

Choose a reason for hiding this comment

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

'body: Opportunity' - Is this right approach to write?

Copy link
Member

Choose a reason for hiding this comment

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

body is good, but to be more specific it should be opportunity

Copy link
Member

@momaqbol momaqbol left a comment

Choose a reason for hiding this comment

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

looks great, please read the requested changes and let's discuss any confusing concepts here or in the meeting. thanks 🚀

@@ -102,7 +102,12 @@ const Projects = {
}

const Opportunities = {
list: () => requests.get<Opportunity[]>('opportunities'),
list: (params?: URLSearchParams) => requests.get<Opportunity[]>('opportunities', params),
post: (body: Opportunity) => requests.post<Opportunity>('opportunities', body),
Copy link
Member

Choose a reason for hiding this comment

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

body is good, but to be more specific it should be opportunity

list: () => requests.get<Opportunity[]>('opportunities'),
list: (params?: URLSearchParams) => requests.get<Opportunity[]>('opportunities', params),
post: (body: Opportunity) => requests.post<Opportunity>('opportunities', body),
getCount: () => requests.get<Object>('opportunities/count'),
Copy link
Member

Choose a reason for hiding this comment

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

I think we're going get number rather than Object

list: (params?: URLSearchParams) => requests.get<Opportunity[]>('opportunities', params),
post: (body: Opportunity) => requests.post<Opportunity>('opportunities', body),
getCount: () => requests.get<Object>('opportunities/count'),
getDetails: (id: string) => requests.get<Opportunity>(`opportunities/${id}`),
Copy link
Member

Choose a reason for hiding this comment

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

id should be number

post: (body: Opportunity) => requests.post<Opportunity>('opportunities', body),
getCount: () => requests.get<Object>('opportunities/count'),
getDetails: (id: string) => requests.get<Opportunity>(`opportunities/${id}`),
update: (id: string, body: Opportunity) => requests.put<Opportunity>(`opportunities/${id}`, body),
Copy link
Member

Choose a reason for hiding this comment

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

id should be number

Copy link
Member

Choose a reason for hiding this comment

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

body is suppose to have all the interface properties set to optional because when send a put request to an api you only want to change specific fields and not everything.

check this out, maybe you'll learn something great with these utility types
https://www.typescriptlang.org/docs/handbook/utility-types.html#partialtype

getCount: () => requests.get<Object>('opportunities/count'),
getDetails: (id: string) => requests.get<Opportunity>(`opportunities/${id}`),
update: (id: string, body: Opportunity) => requests.put<Opportunity>(`opportunities/${id}`, body),
delete: (id: string) => requests.delete<Object>(`opportunities/${id}`),
Copy link
Member

Choose a reason for hiding this comment

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

id should be number

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hello @Enjoy2Live Thanks for the review. Had some confusions cause in api doc i got ->
Screenshot 2022-06-27 at 10 32 49 PM
id as string

Screenshot 2022-06-27 at 10 33 50 PM
and getCount api is returning object.

Should i still proceed?

Copy link
Member

Choose a reason for hiding this comment

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

it's a number

Copy link
Member

Choose a reason for hiding this comment

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

also the id is technically a number but it's not a huge deal if you keep it string

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

Successfully merging this pull request may close these issues.

2 participants