-
Notifications
You must be signed in to change notification settings - Fork 14
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
PoC for map zoomies options #514
Conversation
Your demo site is ready! 🚀 Visit it here: https://ramp4-pcar4.github.io/story-ramp/poc-zoomies/#/en/00000000-0000-0000-0000-000000000000 |
a8f23e7
to
33bbde8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All in all pretty cool.
The third one with scroll watching the home/point-of-interest isn't reacting that well to scrolling the page. I find often I had to scroll well into the POI trigger zone, then scroll back up a pixel to get it to trigger.
The highlight on that one is also pretty big and has noticeable delay trying to draw itself. Can prob optimize the geometry if we go with that solution.
Reviewable status: 0 of 13 files reviewed, 1 unresolved discussion (waiting on @yileifeng)
src/components/panels/interactive-map.vue
line 160 at r1 (raw file):
// zoom out to home extent= const extentSet = rInstance.value.geo.map.getExtentSet(); props.config.duration
slightly cleaner way, if ya wanna
await rInstance.value.geo.map.zoomMapTo(extentSet.fullExtent, null, true, props.config.duration || undefined)
33bbde8
to
c07323c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can adjust the POI intersection observer threshold value to get better scroll trigger behavior if needed for this option
Reviewable status: 0 of 13 files reviewed, 1 unresolved discussion (waiting on @james-rae)
src/components/panels/interactive-map.vue
line 160 at r1 (raw file):
Previously, james-rae (James Rae) wrote…
slightly cleaner way, if ya wanna
await rInstance.value.geo.map.zoomMapTo(extentSet.fullExtent, null, true, props.config.duration || undefined)
Donethanks
Previously, yileifeng (Yi Lei Feng) wrote…
you can do the same thing in the |
c07323c
to
07f5098
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 13 files reviewed, 1 unresolved discussion (waiting on @james-rae)
src/components/panels/interactive-map.vue
line 160 at r1 (raw file):
Previously, james-rae (James Rae) wrote…
you can do the same thing in the
Add the new highlight in
block above
Donethanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like option 3, I didn't have any issues triggering the zoom in but zooming back out only worked if you started letting the map scroll away and went back to it. I like that the motion only occurs on user input, and when the user is reading text the map would be stationary.
For some reason I found option 2 disorienting/caused motion sickness. Maybe a slower zoom out and/or a longer pause in between. Option 1 didn't have the same issue but the continuous motion was still distracting from the text.
Reviewable status: 0 of 13 files reviewed, 1 unresolved discussion (waiting on @james-rae)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also a fan of option 3.
For the issue with the reactivity when scrolling the page: The intersection observer currently triggers when 60% of a point of interest container is visible on the screen, and in order for it to be able to be triggered again the container needs to become less than 60% visible before scrolling back up. Easiest fix for the reactivity in this specific situation would be to simply make the point of interest container taller (whether that's adding padding, or if the actual text content is much longer than this demo that would probably do it as well). The threshold can be tweaked but we would need to make sure it doesn't break other products.
Reviewable status: 0 of 13 files reviewed, 1 unresolved discussion (waiting on @james-rae)
07f5098
to
ad9324d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the first two options from the demo page. For the issues in triggering the intersection observer we can go with Ryan's solution of adding custom CSS to the POI container on the actual TCEI product and hopefully that resolves issues.
Reviewable status: 0 of 13 files reviewed, 1 unresolved discussion (waiting on @james-rae)
ad9324d
to
d1f99ac
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed code related to zoom loop animation and interactive CSS. Should be ready for merge now after demo page sanity test
Reviewable status: 0 of 13 files reviewed, all discussions resolved (waiting on @james-rae)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 6 of 13 files at r1, 1 of 1 files at r3, 1 of 1 files at r4, 5 of 5 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @yileifeng)
Related Item(s)
https://github.com/ramp4-pcar4/tcei-tmx-cwa-storylines/issues/86
Changes
Notes
Will need to clean up this PR before merge to remove excess code and demo slides.
Testing
Demo slides for interactive map with zoom animations
This change is