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

Fix protest list bug #43

Merged
merged 17 commits into from
Oct 8, 2020
Merged

Fix protest list bug #43

merged 17 commits into from
Oct 8, 2020

Conversation

daganomri
Copy link
Contributor

@daganomri daganomri commented Oct 6, 2020 •
edited
Loading

fixes #42, #55

@vercel
Copy link

vercel bot commented Oct 6, 2020 •
edited
Loading

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/1km/1km/ko3keeii1
✅ Preview: https://1km-git-fix-protest-list-bug.1km.vercel.app

@uriklar uriklar requested a review from guytepper October 6, 2020 08:50
@daganomri daganomri mentioned this pull request Oct 6, 2020
Closed
guytepper
guytepper requested changes Oct 7, 2020
View reviewed changes
Copy link
Owner

@guytepper guytepper left a comment

Choose a reason for hiding this comment

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

Thanks! :)
Can you explain what was the issue and how you resolved it?

Also, check line 145 (and ProtestList too) - loading is being passed as a prop.

src/App.js Show resolved Hide resolved
@guytepper guytepper added the reviewed The pull request has been reviewed and waits for the contributor changes / merge. label Oct 7, 2020
@daganomri
Copy link
Contributor Author

daganomri commented Oct 7, 2020 •
edited
Loading

I guess I missed some of the loadings 😅
Basically the loading state made it so protests are only fetched on initial load. After adding the change address button that was no longer necessary.

I can edit this tonight, or maybe you'll prefer just finishing it yourself and merging?

@daganomri
Copy link
Contributor Author

On second thought, the solution could just be dispatching loading: true when changing address. The modal is already in the dispatch context so this should be easy.

@daganomri
Update Modal.js
45adeb7 
I'm editing from my phone. Should theoretically work
@daganomri
Copy link
Contributor Author

I edited from my phone, should work but I didn't test it.

@daganomri
Fixed bug and improved performance
4c1676a 
I realized that having multiple dispatches means multiple renders and multiple useEffect calls.
I combined some reducer cases in order to use dispatch less times.
guytepper
guytepper reviewed Oct 7, 2020
View reviewed changes
src/App.js Outdated Show resolved Hide resolved
@guytepper guytepper merged commit 62a8704 into guytepper:master Oct 8, 2020
@guytepper
Copy link
Owner

I'm really sorry for the delay. Great work 🙂

@guytepper
Copy link
Owner

guytepper commented Oct 8, 2020 •
edited
Loading

+HUGE performance boost. Here's a medal for you 🏅

@daganomri daganomri mentioned this pull request Oct 8, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guytepper guytepper guytepper requested changes

Assignees
No one assigned
Labels
reviewed The pull request has been reviewed and waits for the contributor changes / merge.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

If you use device location and then change the address manually, the protest list doesn't change
2 participants
@daganomri @guytepper