Closed khaister closed 3 years ago
I think this seems good! I think we should keep #656 open though, I think this should work the opposite way as well.
Happy to tackle "the opposite way" too. What did you mean by that thou? :)
Happy to tackle "the opposite way" too. What did you mean by that thou? :)
Ahhh revisiting with closer eyes, I think it does work in that we highlight the card if you click a pin. We don't scroll to it though. I think we could do that, but can still happen in a follow up PR 👍️
@skalnik Ah that makes sense! I thought I have the code to find the actual card and .scrollIntoView
it. Could you please provide which location you were viewing so I can try to debug it?
Hm, if I just go to 94103
and click any site in the map, I don't get the card scrolled into view. Doesn't work for me in either Brave or Safari 🤔
If you do play we scrolling, we should think about how this will work with https://github.com/CAVaccineInventory/site/pull/906 on /embed.
Ooo fancy scrolling!
Whats our plan for this on mobile? I'm worried that the scrolling doesn't make sense because it means that if the user clicks any of the pins on the map, they will immediately be scrolled away from the map (and thats kind of disorienting).
@skalnik I believe I addressed that issue where you click on a marker and the card isn't scrolled. I notice that there isn't always a site card even if there's a marker on the map (I choose the "show all" location filter); I check this by selecting a marker and Ctrl+F the same location but no card for it on the left.
@alexcole Thanks! And I agree with your point on mobile experience. I think we shouldn't have it scrolled at all.
Playing around with devtools and looks like the map is placed at the top when window.innerWidth
< 1024. I think it's safe to assume that when user is on mobile or devices with screen width less than 1024 we can disable the auto scrolling. Thoughts?
@khaister ah sorry i think i have mislead you, I was just trying to share the info :) I am worried that as done it will pull in a bunch of deps. Per their docs: "To avoid this, we recommend using a tool like babel-plugin-preval to generate a static version of your configuration at build-time"
I'm sorry, but I think just using the original constant is fine. Getting babel-plugin-preval to work seems like it would be a lot more work.
@al63 No worries! I'll just revert my last commit then 🙂
Closing in favor of #934.
Addresses #656
This should also work for embedded. ~But I haven't tested it (need help with testing).~
Link to Deploy Preview: https://deploy-preview-897--vaccinateca.netlify.app/
Manual Testing (QA)
(Note: Use your best judgement for time management. If this PR is a minor content adjustment, you might not invest in the full list of QA checks below. But for medium-large PRs, we recommend a thorough review.)
Open the deploy preview and manually perform the following tests with the browser's developer console open. Fix or log any unexpected errors you see in the console. Check off the following manual tests as you go through them. Make sure to resize the screen and check for poorly positioned or spaced items at mobile and tablet sizes (60%+ of our audience is on mobile devices).
Homepage
/near-me
County Vaccination Site List page
County Policies page
Other pages
Did you remember to: