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

PoC for map zoomies options #514

Merged
merged 1 commit into from
Dec 18, 2024
Merged

Conversation

yileifeng
Copy link
Member

@yileifeng yileifeng commented Dec 5, 2024

Related Item(s)

https://github.com/ramp4-pcar4/tcei-tmx-cwa-storylines/issues/86

Changes

  • [FEAT] added option to add CSS classes to background images
  • [FEAT] added config option to specify zoom point details for map panel to perform loop animation
  • [FEAT] added option to zoom interactive map out to base extent + a duration config option for zoom animation
  • PoC with slides containing static image + CSS animation, RAMP instance with zoom API call on loop, interactive map with zoom to point of interest

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 Reviewable

@yileifeng yileifeng added the PR: Frontend PR that primarily involves frontend changes. UI experts and CSS Wizards are asked to review. label Dec 5, 2024
Copy link

github-actions bot commented Dec 5, 2024

Copy link
Member

@james-rae james-rae left a 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)

Copy link
Member Author

@yileifeng yileifeng left a 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

@james-rae
Copy link
Member

src/components/panels/interactive-map.vue line 160 at r1 (raw file):

Previously, yileifeng (Yi Lei Feng) wrote…

Donethanks

you can do the same thing in the Add the new highlight in block above

Copy link
Member Author

@yileifeng yileifeng left a 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

Copy link
Member

@spencerwahl spencerwahl left a 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)

Copy link
Member

@RyanCoulsonCA RyanCoulsonCA left a 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)

Copy link
Member Author

@yileifeng yileifeng left a 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)

Copy link
Member Author

@yileifeng yileifeng left a 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)

Copy link
Member

@RyanCoulsonCA RyanCoulsonCA left a 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: :shipit: complete! all files reviewed, all discussions resolved (waiting on @yileifeng)

@yileifeng yileifeng merged commit 61cb9c8 into ramp4-pcar4:main Dec 18, 2024
3 checks passed
@yileifeng yileifeng deleted the poc-zoomies branch December 18, 2024 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Frontend PR that primarily involves frontend changes. UI experts and CSS Wizards are asked to review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants