search

meetnearme / api

1 stars 0 forks source link

[Feature] New Theme + Early Geolocation / Selection Iteration #113

Closed brianfeister closed 2 months ago

brianfeister commented 3 months ago

This PR implements a new theme (screenshots below) per the mocks from Jeremi Dudu here

There might still be some mis-alignments in terms of theme / styling, feel free to point them out in comments here, but also some of these will evolve as the conversation between Jeremi and I evolves

In addition, I have implemented an Alpine.js + Tailwind combobox UI element (can be componentized later) that does two things:

  1. Shows the implicit geolocation to the user ...we can get this via
    1. Cloudflare Ray ID (which is suffixed with the 3 letter airport code like DEN (Denver)
    2. lat and lon querystring params
  2. Allows the user to search and select from 200 of the largest US cities (selecting an item does nothing, which leads to the open question I'm pondering)
  3. Add /events/{event id} (arguably this should be /event from a REST verb perspective) event detail page with a (temporarily) O(n) query until we get the GSI for event.id in place

Open Question – As always, I'm hyper-paranoid about keeping cost-to-serve low enough that 1 paying user can cover the infra expenses of 1,000+ non-paying users. As such, I'm unsure if we should use the Google Geolocation API in the search bar or just have a static list of "suggestions" for largest cities. This mainly prevents abuse / bots from getting free data out of Google Maps Geolocation API (where we would be paying for / targeted by malicious attackers)

🤔 As I ponder this some more, I'm thinking a list of 200 biggest cities I send down in the HTML response as core suggestions, but then a static / simple string based list on a backend API of our own that has 1,000 biggest cities. That should cover things for early days without sending the full 1000 cities on every home page load

Screenshots

image image image image
brianfeister commented 3 months ago

@Brandon-G-Tripp the cf ray code is already covered here (also cf ray is not something that is modified in this PR, just leveraging the already-existing http header a bit more): https://github.com/meetnearme/api/blob/c1a4ae17de4f2b22a5df4912fbc91cc366b44a60/functions/gateway/handlers/page_handlers_test.go#L48

I've fixed the failing tests and renamed the function to GetEventById() per request: https://github.com/meetnearme/api/pull/113/commits/d322a39cba2786cee33891f44c2e703884c039f5

Until we get int UI testing / validation (which I think we'll do with jest eventually), a change like this one doesn't do much in terms of code coverage change because it's mostly UI aside from the GetEventById() which I've covered

Let's keep things simple and say that the code coverage of 50% is our standard. If a PR drops us below that, we'll fix. Otherwise, we won't worry about it.

A separate conversation we can have is whether we want to submit PRs to increment coverage upwards (say we hit 55% and decide to lock at that threshold), but I think this discussion should be out of scope for the current PR