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

331 pin on click #472

Merged
merged 15 commits into from
Jun 4, 2022
Merged

331 pin on click #472

merged 15 commits into from
Jun 4, 2022

Conversation

gsehrlich
Copy link
Collaborator

@gsehrlich gsehrlich commented May 27, 2022

Checklist

  • Add description
  • Reference the open issue that the pull request addresses
  • Pass code quality checks
    • spin up docker docker-compose up -d --build
    • enter api container docker-compose exec api /bin/bash
    • run api tests make validate
    • exit container ctrl/command+D or exit
    • enter web container docker-compose exec web /bin/sh
    • run front-end tests npm run test or npx jest
    • lint npm run lint-fix
    • exit container as above
  • Request code review
    • Please allow 36 hours from opening a pull request before merging a pull request- even if it has already received an approving review.
  • Address comments on code and resolve requested changes
  • Merge own code

Description

Issues: #331, #442, and #464

Brief description of solution

  • Manually handle mouse interaction with map so that the popup follows the cursor while hovering over data and can be pinned by clicking on a data point
  • Add clickable question mark link to popup
  • Use react-map-gl's Popup element to fix the popup to a specific latitude and longitude; popup now automatically appears in optimal orientation to stay within the map frame
  • Give the popup a custom close button that plays nice with the new event handling
  • Splice in correct EPA link
  • When the user hovers over the question mark, show a modal at the top of map to clarify units and EPA link

Reviewer: It'll be easier to understand my edits by looking at the individual commits. At least for the last batch of commits (today's), I separated them into hunks representing atomic changes.

@gsehrlich gsehrlich requested a review from theecrit May 27, 2022 04:35
@theecrit
Copy link
Collaborator

theecrit commented Jun 2, 2022

@gsehrlich Should we start with this link for the question mark icon destination? https://www.airnow.gov/aqi/aqi-basics/

It's imperfect, because it implies we're measuring more than just PM2.5, but I think it's a decent MVP start.

I also wonder if we should change the icon from a question mark to an off-site link icon. Normally I would discourage opening links in a new window, but given the nature of the interaction (that users will likely want to remain in the map experience), I wonder if we should make an exception in this instance. E.g. "external square alternate" from Semantic UI.

@gsehrlich
Copy link
Collaborator Author

gsehrlich commented Jun 2, 2022 via email

@theecrit
Copy link
Collaborator

theecrit commented Jun 2, 2022

My main concern is
actually that users will think the numbers on the map (which are in hidden
units of micrograms per cubic meter) mean the same thing as the numbers in
the chart (which have been converted to AQI).

Yeah, I hear that. Not sure we should worry about it for MVP but maybe prioritize it as a fast-follow for improvements?

@theecrit
Copy link
Collaborator

theecrit commented Jun 2, 2022

Actually, I'm just realizing, we have an open criteria item in the comments on this in-progress issue: #464 (comment)

@gsehrlich
Copy link
Collaborator Author

gsehrlich commented Jun 3, 2022 via email

Copy link
Collaborator

@theecrit theecrit left a comment

Choose a reason for hiding this comment

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

LGTM!

@gsehrlich gsehrlich merged commit a783af6 into develop Jun 4, 2022
@gsehrlich gsehrlich deleted the 331_pin-on-click branch June 4, 2022 20:11
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