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

Add current satellite loop #1310

Merged
merged 9 commits into from
Aug 1, 2024
Merged

Add current satellite loop #1310

merged 9 commits into from
Aug 1, 2024

Conversation

greg-does-weather
Copy link
Collaborator

@greg-does-weather greg-does-weather commented Jun 10, 2024

What does this PR do? 🛠️

  • adds WFO grid information to the page-level metadata (available as weather.grid in Twig templates)
  • fetches WFO satellite metadata from NESDIS (the WFO is part of the URL)
  • builds correct GIF URL based on satellite metadata
  • adds the most recent satellite loop GIF for the location's WFO as an image in the today tab

Deployed in my environment: https://weathergov-greg.app.cloud.gov/point/36.166/-86.779?#current

Screenshots (if appropriate): 📸

image

@greg-does-weather greg-does-weather changed the title add current satellite loop Add current satellite loop Jun 17, 2024
@greg-does-weather greg-does-weather force-pushed the mgwalker/satellite branch 2 times, most recently from 973aef6 to 996425f Compare July 29, 2024 21:35
@greg-does-weather greg-does-weather marked this pull request as ready for review July 30, 2024 15:20
Copy link
Collaborator

@partly-igor partly-igor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's looking good, a few design observations:

  1. The "Satellite" heading isn't showing up a "visually h2" I think because it's missing the wx prefix
  2. The satellite gif should take up 100% width of the container. Currently it varies depending on the browser width
  3. The most complicated issue (perhaps meriting its own PR) is that we need to allow users to stop the gif for general usability but also for accessibility standards. I've put together documentation in figma for how we could do this (along with the notes above). I've put the icons for the new buttons (play, pause) on the drive

@greg-does-weather
Copy link
Collaborator Author

I think you're right that stopping animation is a separate PR because there are a few things to consider with it. I'll write out my thinking here so I don't have to try to just keep it in my head.

There's no way to pause a GIF in the browser, and there are basically three options to support pause-like functionality:

  1. When the image should be paused, replace it with a static image. When it should be playing, replace it with the GIF. This is an easy approach, but if the user "pauses" the GIF in the middle of the loop, instead of staying at and resuming from the middle, they'll instead see a flash as the image changes to the static image, and when they "unpause" the GIF, it will start playing from the beginning, not where they stopped it.
  2. Replace the GIF with a series of images and animate them ourselves. This is harder and I'd advocate against it, but it does allow us to preserve the right tracking location within the animation. I think we already have access to the individual image frames via the metadata JSON, so at least we don't have to figure that out. Another argument against this approach is it requires the user's device to download each of the images individually, which could result in a lot more data going across the wire than downloading a single GIF.
  3. We could talk to NESDIS about whether they could add a WebM (or similarly patent unencumbered encoding) video file in addition to the GIF. Oftentimes video files can be smaller than GIFs, and because they are videos, we can take advantage of browser-native video controls, including pausing and resuming. For example, the GOES16 GIF for MPX right now is about 7.7 MB. Converting it to WebM reduces that to 1.3 MB. (I don't want to make too many assumptions about how NESDIS is creating images, but ffmpeg can convert GIFs to WebM.)

@greg-does-weather
Copy link
Collaborator Author

For the width of the GIF, the source image is 600 pixels wide. Should we stretch the image to be 100% of the container? (Easy fix, just wanted to confirm!)

@partly-igor
Copy link
Collaborator

Yeah that makes sense – I do think we should pursue one of these options, and it sounds like option 3 would be the best path if we are able to get the video format. I am curious if converting the gif to WebM on our server is a possibility?

For the width – yeah, with a GIF I tend to be more forgiving about going a little bigger than the source size, and I think it'll look better than the variable border. I do want to make sure the aspect ratio stays the same (i.e stretch proportionally) though.

Copy link
Contributor

This PR modifies theme Javascript or CSS assets but does not update the theme libraries file. Did you mean to update the appropriate version information in the libraries file?

@greg-does-weather
Copy link
Collaborator Author

Yeah, fully agreed. If we can get video, that's the best case. I'll open a ticket to dive deeper into the pause/play issue.

For now, I've updated this PR to make the satellite's minimum width 100%.

Copy link
Contributor

github-actions bot commented Aug 1, 2024

This PR modifies theme Javascript or CSS assets but does not update the theme libraries file. Did you mean to update the appropriate version information in the libraries file?

Copy link
Contributor

github-actions bot commented Aug 1, 2024

This PR modifies theme Javascript or CSS assets but does not update the theme libraries file. Did you mean to update the appropriate version information in the libraries file?

Copy link
Collaborator

@partly-igor partly-igor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good, one small question about the alt text, but otherwise 👍

Should we open a ticket for the animation controls?

</div>

<div aria-hidden="true">
<img src="{{ weather.satellite.gif }}" class="wx-minw-full">
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From some light searching, it seems like even though it's aria-hidden we should have a blank alt attribute on this.

I am wondering if there is a reason not to use alt text here instead of the sr-only div/paragraph above?

@greg-does-weather greg-does-weather merged commit b47e41c into main Aug 1, 2024
17 of 18 checks passed
@greg-does-weather greg-does-weather deleted the mgwalker/satellite branch August 1, 2024 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prototype: Satellite
2 participants