Skip to content

Conversation

@Akashdeep-Patra
Copy link
Contributor

constants and utils package added, a wrapper for axios request added

Copy link
Contributor

@swarajpure swarajpure left a comment

Choose a reason for hiding this comment

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

I guess it'd be better to have default value for method and responseType as well since most of the requests on the frontend will be GET and response type of most requests would be json only

What do you think @Akashdeep-Patra? 🤔

@Akashdeep-Patra
Copy link
Contributor Author

@swarajpure axios already has a default value for method i.e get, and i can put a json default for responseType?

@harshith-venkatesh
Copy link
Contributor

Hi @Akashdeep-Patra , I had a small request, There was a bad user experience observed when a user tries to perform send/receive operation multiple times due to declaring the value of receiver and currency type as null on close modal.
image

@Kratika0907 @swarajpure I have requested @Akashdeep-Patra in the call to make this change and include in this PR.

Thanks @Akashdeep-Patra for the quick fix


const closeModal = () => {
showModal((prev) => !prev);
setReceiver('');
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the quick fix

timeout: 1000,
});

export const makeRequest = (obj) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

proper naming obj can be renamed as requestConfig

Copy link
Contributor Author

Choose a reason for hiding this comment

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

naming requestObject because it will also have other info i.e data, and config is a bit misleading

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.

4 participants