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

Add feature to show popup for request in URL param #71

Merged
merged 4 commits into from
May 23, 2020

Conversation

piratefsh
Copy link
Collaborator

Summary

Allow /delivery-needed to receive a request code via URL param and to display popover for corresponding request (if it exists).

e.g. http://localhost:3000/delivery-needed?request=MD4VL2 will open the app with the popover for MD4VL2 open.

Screen Shot 2020-05-15 at 4 46 23 PM

If the request cannot be found, it will display a warning alert with a link to return to the default view. This is to hopefully prevent confusion for folks looking at the map for a claimed delivery/
Screen Shot 2020-05-15 at 5 36 08 PM

This can be used for linking the request on the map in the Slack app, making it easier for delivery volunteers to look locations up on the map.

Limitations

  • Opening the popover does not zoom into the location. Default map viewport will always include all open delivery requests
  • If there are multiple requests at the same coordinate, it will only show that one request. I decided not to spend time implementing this feature to show all requests at a location for now, but can revisit if needed.

const [popup, setPopup] = useState();

// display popup if request code is present in URL search param
useEffect(() => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

have to wrap this with useEffect so it only fires once the layers have been mounted.

Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need useEffect? don't we have all the info on render if paramRequest is truthy?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not 100% sure on the details of this but if you remove the useEffect wrapper, React just tries to rerender the component over and over again, resulting in Error: Too many re-renders. React limits the number of renders to prevent an infinite loop. I think it's because we are updating the state during render, versus after render if we useEffect. EIther way when working with maps, I have found it to be best to let layers render first before imposing state on it to prevent an unexpected sequence of events.

"bottom-left": [12, -38],
bottom: [0, -38],
"bottom-right": [-12, -38]
"bottom-left": [6, -19],
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

just fixing offset so popover is closer to the marker.

@piratefsh piratefsh requested a review from mjmaurer May 15, 2020 21:43
Copy link
Contributor

@mjmaurer mjmaurer left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Sorry for the delay. Only one comment.

const [popup, setPopup] = useState();

// display popup if request code is present in URL search param
useEffect(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need useEffect? don't we have all the info on render if paramRequest is truthy?

@piratefsh piratefsh force-pushed the piratefsh/add-url-param-search branch from 4375200 to 5d6a1dd Compare May 22, 2020 21:54
@mjmaurer mjmaurer merged commit 6d6f041 into master May 23, 2020
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

2 participants