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

uses mapbox-gl and shows all upcoming events #29

Closed
wants to merge 8 commits into from

Conversation

mick
Copy link

@mick mick commented Sep 30, 2019

Mapbox gl allows for client side rendering of map data, which with a lot of custom markers allows for better performance over leaflet.

This PR relies on the pre-fetched bundle of events in #27. This does change a bit of the map / marker logic. There is a gh-pages deployed version here: https://mick.github.io/eventmap/

Screen Shot 2019-09-30 at 1 19 22 PM

Screen Shot 2019-09-30 at 1 19 39 PM

TODO

  • handle zipcode entry (center on zipcode or on nearest event to that zipcode?)

cc @jasonkalmeida @jlev

@jlev
Copy link
Contributor

jlev commented Oct 1, 2019

Code looks great, but I wonder about the user experience of seeing a few early states really busy, and basically nothing elsewhere. Is this better than asking people to put in their location first, and then showing them relevant local or virtual events?

Of course the event distribution will change as the campaign moves on from Iowa, but we'll want some way for people across the country to feel engaged.

@mick
Copy link
Author

mick commented Oct 1, 2019

@jlev That is a good question. The focus of events is going to be in the early primary states at this point in the campaign. Personally I thought it was great to see events in places like Montana and Alaska, places that aren't close to me so I wouldn't have searched for them. And you're right the distribution of events will change rapidly.

I do think you make a great point that we should direct people to virtual events and volunteering to host events themselves. Maybe we can put something like this under the zipcode input / events list:

Don't see an event near you? Join a virtual event or host your own event!

@jasonkalmeida
Copy link
Collaborator

On the topic of redirecting users to create events, we have a technical requirement here to put up a prompt to users if there are no events in their area: #6

@jasonkalmeida
Copy link
Collaborator

Would it be possible, on load before a user enters a zip code, to have a rotating marker fading in and fading out of different locations on the map in areas that have events?

The fading in and out marker will indicate events happening all over the country, and given that it’ll constantly be changing it doesn’t lead to the perception of “only in x states”.

It might also mitigate frontend performance to just be modifying one (or we could have a couple) marker.

@joegoldbeck
Copy link

joegoldbeck commented Oct 3, 2019

My 2 cents:

I think all the styling points here are good ideas! The fading in and out before you type in a zip code seems neat.

I also think starting with the full map of events is a pretty dope starting point though :D. I think it's a better initial experience than a blank map imo. I like the look of seeing all the mapped events, personally (though would be good to start zoomed on USA if possible). However, the zip code lookup appears broken in https://mick.github.io/eventmap/ When I type in a zip code, it doesn't change to show much just those nearby events, nor does it zoom me in

Strong vote to get to shippable MVP asap! There are volunteers out there who are missing a map-based event search ;)

@jasonkalmeida
Copy link
Collaborator

Hey, so reviewed the code - and I think I need to go over it again 😅

This PR changes code in multiple components that can possibly impact other features (such as search which is currently broken) and features that are currently being built by other branches. Additionally, I think there are impacts that we can’t visually see right now. For example, I think this breaks the hover-highlight (hovering over an event list item and highlighting the respective venue marker).

I totally can concede that the starting code base can be drastically improved, so I just want to go through this again and better understand the pros and cons of the changes made/their impact. But as a result, I don’t feel comfortable merging this today.

…ch, add link to virtual events and creating new events
@mick
Copy link
Author

mick commented Oct 4, 2019

Thanks for the comments and feedback everyone! I just committed some more changes and updated https://mick.github.io/eventmap/

I also think starting with the full map of events is a pretty dope starting point though

I agree. The perf shouldn't a problem with this version of map. So I think we should start with showing all events.

we have a technical requirement here to put up a prompt to users if there are no events in their area: #6

👍 I have the event list now show only the events that are in current map viewport. I added links to virtual events and creating an event at the bottom of the list. So if there are few or no events in the area the user will see it. like:

Screen Shot 2019-10-03 at 6 17 07 PM

However, the zip code lookup appears broken in https://mick.github.io/eventmap/ When I type in a zip code, it doesn't change to show much just those nearby events, nor does it zoom me in

Yeah I broke that and posted this PR to get feedback on the direction before spending time fixing it. It's reimplemented now. I used the mobilizeamerica api just to get the nearest event to a zipcode and then center the map there.

(though would be good to start zoomed on USA if possible)

@joegoldbeck Can you say more here? It should be roughly centered on the USA at zoom level 3 right now. It does remember your location in the url hash. So you can link to a location and if you refresh you'll be in the same location.

It could be more zoomed out to start. It could be initially set fit the bounds of the event data.

For example, I think this breaks the hover-highlight (hovering over an event list item and highlighting the respective venue marker).

This should also be fixed now. Although the reverse needs to be clearer (click on a map marker and see the event details). I'll look into that.

This PR changes code in multiple components that can possibly impact other features (such as search which is currently broken) and features that are currently being built by other branches.

@jasonkalmeida Yeah that changes here touch everything. But there are not any other PRs open. Its unclear to me what is still in progress. Can we open draft PRs for branches that have work that is in progress?

Strong vote to get to shippable MVP asap! There are volunteers out there who are missing a map-based event search ;)

Agreed! I've updated https://mick.github.io/eventmap/ Please try it again and list any issues you run into. The biggest outstanding issue is making this useable on mobile #15

@joegoldbeck
Copy link

joegoldbeck commented Oct 4, 2019

@mick This looks great! But I am actually seeing significant performance issues. See attached video:
Slowness.mov.zip

WRT the zoom level. It looks better now but when I checked the other day, I saw the whole globe wrapped around :p

…ing fewer events in list, and using mapbox-gl feature state rather than updating source data. Also highlight and scroll into view an event in the eventlist when clicking on the map
@mick
Copy link
Author

mick commented Oct 5, 2019

@joegoldbeck whoa yeah that's bad. I just tried it out in Safari and perf was even better than Chrome for me so I'm sure what is up. But there were several things I could do it make it better. I just committed 3 improvements in 111ff45 Can you try that out? I've updated https://mick.github.io/eventmap/ as well.

the reverse needs to be clearer (click on a map marker and see the event details). I'll look into that.

I also added this. When you click on a map marker it highlights the event in the list and scrolls that event into view if its not already.

I think this is close to merge-able. Thoughts?

@jasonkalmeida
Copy link
Collaborator

Hey @mick

Getting this error when I query a zip code

Screen Shot 2019-10-04 at 6 06 51 PM

@jasonkalmeida
Copy link
Collaborator

I also added this. When you click on a map marker it highlights the event in the list and scrolls that event into view if its not already.

Does this include multi-event support for a single venue? The current feature on staging is that if a marker is clicked, the event list filters to just the events at that marker - once a user un-selects that marker (by clicking elsewhere/another marker) the event list resets.

@mick
Copy link
Author

mick commented Oct 5, 2019

@jasonkalmeida fixed the zipcode c3d1f48

Does this include multi-event support for a single venue?

It doesn't as explicitly. When clicking on a marker it centers and zooms in. The list is narrowed down to what's in the viewport of the map. It highlights (just like hover state) and scrolls to the next event at that location.

Try it out and lmk if it makes sense. Now that the list is dynamic based on the viewport, it made sense to me. But I can revert that.

@jasonkalmeida
Copy link
Collaborator

Hey Mick!

Sorry for the delays between replies. So a couple things here:

  • I do think filtering by marker is super important - the reason for this is because a single marker (representing a venue) might have multiple events and the best way we have to convey that to a user is to have an marker based filter. I believe your current solution is to scroll the list to the first event of the marker, which probably does include any other events right under, but given that it is still in the long list of items it isn’t 100% clear to the user. I also agree that our current implementation that is live isn’t the gold standard, so I want to figure out something that is better on this front (possibly text based info when a filter is active).

  • I’m running into this weird bug where if my mouse hovers below the top level event, the list just scrolls all the way to the bottom. I’m sure this is probably a small bug/easy fix, but just wanted to call it out.

SpeedyClip.zip

  • I occasionally see small performance issues, but nothing like what @joegoldbeck commented with - I'm not sure if he's been able to test again to see if your changes improved performance. This is making me wonder what the longterm performance will be of this on the client side as the total number of events (presumably) grow in the coming months.

@joegoldbeck
Copy link

To chime in on performance, I ran that test when I had a billion windows open. But, I also have a pretty new MacBook. Developers tend to have decent hardware, but users do not. So I think testing it while your computer is overloaded is a reasonable simulation of what a good fraction of real users will experience. (No point in getting fancy with a tool like this if it’ll make it unusable for even 10% of users IMO)

If there have been performance improvements, I can try to test again.

@joegoldbeck
Copy link

Ok, it's definitely faster now!

@joegoldbeck
Copy link

joegoldbeck commented Oct 9, 2019

One thing that feels kind of weird UX-wise is when I type in a zip code, it keeps the old event list while it does a zoom-y thing, and only updates it once it's stopped. Kind of jarring because it looks like maybe it's broken while it's doing the movement / personally I want to see my events right away and don't care so much about having a nice slow swoop to the new location.

showmetheevents-sm.mov.zip

mick added 2 commits October 9, 2019 16:40
…ction by click elsewhere on the map. clear results when moving map because of zipcode
@mick
Copy link
Author

mick commented Oct 10, 2019

I do think filtering by marker is super important

@jasonkalmeida cool. I changed back to how it worked before. It still filters by viewport, but when a location is selected (by clicking on the marker) it filters to events at that location. When the user clicks off the marker anywhere else on the map it returns to just showing the events in the viewport.

I also agree that our current implementation that is live isn’t the gold standard, so I want to figure out something that is better on this front (possibly text based info when a filter is active).

Yeah there are a couple ways things are being filtered. Might be worth adding a "clear selection" button above or below the list. what do you think?

Ok, it's definitely faster now!

@joegoldbeck Agreed on the perf goals. Thanks for trying it again. Most of the perf gain came from rendering fewer elements in the event list + no longer updating all the map data when highlighting a marker. It uses setFeatureState which is the newer and much more performant way of setting state on a map feature)

This is making me wonder what the longterm performance will be of this on the client side as the total number of events (presumably) grow in the coming months.

@jasonkalmeida yes there is an upper limit, I tested with random generated points, that limit is ~6000 points in view for me, so probably target half that for most users ~3000. We're at ~800 now. At that threshold we could either use clustering, not show all points until users zoom in, or swap back only showing results from mobilzeamerica api. Maybe while keeping some limited initial state to still convey events across the country.

One thing that feels kind of weird UX-wise is when I type in a zip code, it keeps the old event list while it does a zoom-y thing, and only updates it once it's stopped.

@joegoldbeck yeah totally. I removed the animation and cleared the list on zipcode entry. So it goes blank briefly before it populates with events from the new viewport.

Updated: https://mick.github.io/eventmap/

@joegoldbeck joegoldbeck closed this Mar 8, 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.

4 participants