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.

Sign up for GitHub

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

Jump to bottom

[Feature] Reflect network requests on button #291

Closed
wants to merge 5 commits into from
Closed

[Feature] Reflect network requests on button #291

wants to merge 5 commits into from

Conversation

kunal00000
Copy link
Contributor

@kunal00000 kunal00000 commented Dec 28, 2022 •
edited
Loading

Issue Number

fixes #287

Describe the changes you've made
I have created a new component which has loader on button itself that loads onclick.
Added this component to all the Button with async callbacks.

Describe if there is any unusual behavior (Any Warning) of your code(Write NA if there isn't)

  1. In some asynchronous functions preventDefault was used which was restricting the new button component as expected. So, I removed it. No extra warnings or errors.
  2. I changed apiUrl to localhost:1337 , do I need to change it to previous one for pr.

Additional context (OPTIONAL)

Screenshot 2022-12-29 at 4 16 25 AM

Screenshot 2022-12-29 at 4 18 28 AM

Test plan (OPTIONAL)

A good test plan should give instructions that someone else can easily follow.

Checklist

  • [x ] My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • All new and existing tests passed.
  • [x ] The title of my pull request is a short description of the requested changes.

Provide a Deployed link of route/page that needs to review

Preview: Deploy preview link here with the appropriate route

@kailash360
Copy link
Member

You need to add back the config.temp.json file. It is the file to refer the required config. You should create a config.json file inside the ./src directory and define your configurations there corresponding to the config.temp file.

@kunal00000
Copy link
Contributor Author

kunal00000 commented Dec 29, 2022 •
edited
Loading

Oh ok thanks, added config.temp.json file back to src folder.
@kailash360 you can review it now.

@kunal00000
Copy link
Contributor Author

Can I solve this #201 issue also in this pull request?

@kailash360
Copy link
Member

Can I solve this #201 issue also in this pull request?

You can raise a different PR for that issue. Let this PR be dedicated to network request on buttons.

@kunal00000
Copy link
Contributor Author

@cyntss Please review this pull request.

@kailash360
Copy link
Member

@kunal00000
Can you resolve the reviews so that this PR can be merged?

@kunal00000
Copy link
Contributor Author

@kailash360
Yes, I added back config.temp.json as you said in previous review. If I get more reviews to solve, I would solve them.

@cyntss
Copy link
Member

cyntss commented Jan 13, 2023

I see that @kailash360 is already reviewing this PR. Thank you Kailash. You can merge it if you think it follows all our guidelines

@kunal00000
Copy link
Contributor Author

@kailash360 , you can merge this pr I have resolved the reviews.

kailash360
kailash360 approved these changes Jan 31, 2023
View reviewed changes
Copy link
Member

@kailash360 kailash360 left a comment

Choose a reason for hiding this comment

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

LGTM!

src/components/ButtonWithLoader.js
setLoading(false)
}
return (
<button onClick={handleClick} disabled={isLoading} {...rest}>
Copy link
Member

Choose a reason for hiding this comment

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

for the disabled attribute, we should also consider the declaration of the attribute where the component is used.

Copy link
Member

Choose a reason for hiding this comment

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

@kunal00000 resolve this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you please elaborate the motive of doing so because I think it should be disabled in all cases whenever loading is happening and this is happening through this code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So personalising it everywhere used would not benefit. If some alteration is needed please give your take on this.
@kailash360

@kunal00000 kunal00000 closed this by deleting the head repository May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kailash360 kailash360 kailash360 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature] Reflect network requests on button
3 participants
@kunal00000 @kailash360 @cyntss