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

Concurrent Incidents #494

Merged
merged 4 commits into from
Aug 4, 2020
Merged

Concurrent Incidents #494

merged 4 commits into from
Aug 4, 2020

Conversation

bliitzkrieg
Copy link
Contributor

Overview

Added ability to toggle concurrent incidents on map inside Incident analysis view

GitHub Issues

Changes

  • N/A

Screenshots / Videos

Screen Shot 2020-06-16 at 8 56 06 PM

Screen Shot 2020-06-16 at 8 56 09 PM

Steps to Test

  • Find incident in incident analysis with concurrent incidents
  • Scroll to Map
  • Click Layer button to toggle them on
  • Search around map to find concurrent incidents

@bliitzkrieg bliitzkrieg requested a review from chopchop505 June 17, 2020 01:02
@bliitzkrieg bliitzkrieg self-assigned this Jun 17, 2020
@bliitzkrieg
Copy link
Contributor Author

bliitzkrieg commented Jun 17, 2020

@chopchop505 Should be good for review. Do you want to use a different icon for the toggle?

@larskendall larskendall self-requested a review July 16, 2020 23:19
Copy link
Contributor

@larskendall larskendall left a comment

Choose a reason for hiding this comment

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

Looks great to me overall. I just added some notes for one bug I noticed, along with a couple other minor suggestions.

One other useful addition would be to have an "enabled" visual state for the button, so that you can tell when concurrent incidents are being displayed or not. Right now it's ambiguous. You could try giving it a border value of 2px solid #00a7dc when it's enabled, which should look pretty good I think. For that to work well you'd probably also need to set outline: none in the button's focus state. Otherwise it'll always look like it's enabled when you click it.

@larskendall
Copy link
Contributor

larskendall commented Jul 17, 2020

I noticed one more thing that should probably be adjusted...

Right now there's some default styling for the incident map component that's mixed in the incident analysis page's css (.incident-map-container and .incident-map-concurrent-layer in incident-analysis.scss), but I think that probably belongs in a file specifically for the incident map component, so that it's reusable. Otherwise you'd have to set the styling for that stuff again for any page where you want to reuse the concurrent incidents toggle button.

@bliitzkrieg bliitzkrieg requested a review from larskendall July 27, 2020 21:10
@bliitzkrieg
Copy link
Contributor Author

Should be good for review

@larskendall
Copy link
Contributor

Looks great! I'll merge it in.

@larskendall larskendall merged commit a6a7283 into master Aug 4, 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.

2 participants