Add an option for users to use their browser's geolocation feature #520
Conversation
greg-does-weather
requested review from
eric-gade,
colinmurphy01 and
shadkeene
|
@shadkeene @colinmurphy01 Especially interested in that kind of behavior flow above and whether that makes sense. If it's helpful, we can hop on a call and I can demo it so you get a feel for how it behaves. |
|
Assigning to myself and will take a look next week |
|
@greg-does-weather makes sense to me. Do all browsers support geolocation? |
|
Pretty much all browsers, yes: https://caniuse.com/?search=geolocation And for those that don't, this code (should) hide the button. |
There was a problem hiding this comment.
Code looks good 👍 -- just a couple of inline comments.
Also it looks like the JS confirm dialog does not appear in desktop Safari (the normal browser prompt does, however)
Code Review Checklist
This is an automated comment on every pull request requiring a review. A checked item is either an assertion that I have tested this item or an indication that I have verified it does not apply to this pull request.
The Basics
- Checks are passing
- I read the code
- I ran the code
- (if applicable) Post deploy steps are run
- (if applicable) I validated the change on the deployed version in
Documentation
- changes to “how we do things” are documented in READMEs
- all new functions and methods are commented using plain language
- any new modules added documented in modules.md
Security
- security false positives are documented
- data from external sources is cleaned and clearly marked
Reliability
- error handling exists for unusual or missing values
- interactions with external systems are wrapped in try/except
- functionality is tested with unit or integration tests
- dependency updates in composer.json also got changed in composer-lock.json
Infrastructure
- all changes are auditable and documented via a script
- it is clear who can and should run the script
- (if applicable) diagrams have been updated or added in PlantUML
Accessibility
- New pages have been added to cypress-axe file so that they will be tested with our automated accessibility testing
- Meets WCAG 2.0 AA or 2.1 AA for Section 508 compliance
- Site is keyboard accessible. All interactions can be accessed with a keyboard
- Site is free of keyboard traps. The keyboard focus is never trapped in a loop
- All form inputs have explicit labels
- Form instructions are associated with inputs
- All relevant images use an img tag
- All images have appropriate alt attributes
- Multimedia is tagged. All multimedia has appropriate captioning and audio description
- Text has sufficient color contrast. All text has a contrast ratio of 4.5:1 with the background
- Site never loses focus. Focus is always visible when moving through the page with the keyboard
- Tab order is logical
- Tables are coded properly. Tables have proper headers and column attributes
- Headings are nested properly. Heading elements are nested in a logical way
- Language is set. The language for the page is set
- CSS is not required to use the page. The page makes sense with or without CSS
- Links are unique and contextual. All links can be understood taken alone, e.g., ‘Read more - about 508’
- Page titles are descriptive
Device Matrix
- firefox/gecko (renders correctly and user interactions work)
- chrome/chromium/edge (renders correctly and user interactions work)
- safari/webkit (renders correctly and user interactions work)
- web page is readable and usable
- at 480px (mobile)
- at 640px (tablet)
- at 1024px (desktop)
| ({ code }) => { | ||
| if (code > 1) { | ||
| // There was a problem getting the user's location. They allowed it, | ||
| // but the browser gave us an error. | ||
| // (Error code 1 is for when the user denies access to location, so | ||
| // for our purposes, that is not an error.) | ||
| } | ||
| }, |
There was a problem hiding this comment.
My $0.02 is that in this case, the end user should be informed of this failure somehow (even if it's just an alert() for now)
There was a problem hiding this comment.
Ooh, good call! I'll add that in.
| const goToLocationPage = (latitude, longitude) => { | ||
| // Push the next URL into the history to preserve the current URL in the | ||
| // browser history stack. | ||
| window.history.pushState(null, null, `/point/${latitude}/${longitude}`); | ||
| window.location.href = `/point/${latitude}/${longitude}`; | ||
| }; |
There was a problem hiding this comment.
We might have a chance to spare the server some requests at this point if we make the API call from points -> WFO&grid from the browser instead, and navigating to the actual forecast page from there. Of course, that involves making API requests from the browser for the first time in this application, which is the main tradeoff.
There was a problem hiding this comment.
I'm not opposed. Especially since the server is going to make the call to the /points endpoint again anyway to get the place name. (It's very silly...)
|
I'd like to see if I can figure out what's happening in Safari before merging. It feels minor, but at the same time, we don't really want different user experiences based on browser, to the extent we can prevent it. |
|
Hrm. I couldn't reproduce the weirdness in Safari desktop. 😕 But still, I added code to pass location errors directly to users for now. As you said, we'll want to revisit it later, but this'll get us going. Screen.Recording.2023-12-12.at.2.49.21.PM.mov |
|
@greg-does-weather can you demo this on Friday? |
|
Yep |
What does this PR do? 🛠️
Adds a button users can click to use their browser's geolocation feature instead of typing in a location. The behavior is roughly as follows:
The idea is if the user has not already given us permission, we will first let them know we're going to ask for it and disclose how we'll use that information. Then we ask the browser, which will usually generate a browser-supplied prompt to allow/prevent.
Closes #184
What does the reviewer need to know? 🤔
make ccshould do the job.Screenshots (if appropriate): 📸
The location search component gets a new button:
The prompt if the user has not previously given us permission to use location: