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

fixed mine route issue #386

Merged
merged 5 commits into from
Feb 2, 2023
Merged

Conversation

kshitij430
Copy link
Contributor

@kshitij430 kshitij430 commented Jan 8, 2023

This PR fixes issue #375

There were two issues .

  1. The Mine component was re evaluating in a loop as the useEffect hook had "response" as dependency.
  2. The if condition to check if there is a response with length had a not condition which did the opposite. Ex - for 0 results found !0 would evaluate to 1 and !1 would evaluate to a falsy value due to which the setTasks state updating function was not getting executed.

Fixes I have done -

  1. Removed response as a dependency as we only want to call the API once when the component is mounted.
  2. Removed the condition to check response results as these checks are already done in the JSX return. Ex-> display no results found if length is 0 and otherwise.

@vercel
Copy link

vercel bot commented Jan 8, 2023

@kshitij430 is attempting to deploy a commit to the RDS-Team Team on Vercel.

A member of the Team first needs to authorize it.

@Maheima
Copy link
Contributor

Maheima commented Jan 9, 2023

Please add description to the PR related to why it was happening and how it was fixed

@kshitij430
Copy link
Contributor Author

Please add description to the PR related to why it was happening and how it was fixed

Already Done. Can you please review it?

.env.development Outdated Show resolved Hide resolved
ivinayakg
ivinayakg previously approved these changes Jan 23, 2023
Copy link
Contributor

@ivinayakg ivinayakg left a comment

Choose a reason for hiding this comment

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

perfect

vvaibhavdesai
vvaibhavdesai previously approved these changes Jan 23, 2023
heyrandhir
heyrandhir previously approved these changes Jan 24, 2023
@harshith-venkatesh
Copy link
Contributor

Could you add video to showcase the issue being resolved in different scenarios please @kshitij430

@kshitij430
Copy link
Contributor Author

kshitij430 commented Jan 26, 2023

I have added a video considering where no results are found and where 1 or more are found. Now since I did not have any tasks assigned to me I replicated the behavior by duplicating the response as you can see in the video.

RDS.mp4

Maheima
Maheima previously approved these changes Jan 26, 2023
@harshith-venkatesh
Copy link
Contributor

From contract , response can't be null or undefined or some error?
If we have proper error handling ,we are good with merging

@kshitij430
Copy link
Contributor Author

From contract , response can't be null or undefined or some error? If we have proper error handling ,we are good with merging

I doubt the API will give a null or undefined but In cases if the response might be null or undefined. It is better to check if there is a required response present and then update the state for tasks.
In a scenario if there is an error the jsx return will handle it.

Copy link
Contributor

@Maheima Maheima left a comment

Choose a reason for hiding this comment

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

LGTM!

@Maheima Maheima merged commit 810cc74 into Real-Dev-Squad:develop Feb 2, 2023
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.

None yet

7 participants