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 link preview image #328

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

add link preview image #328

wants to merge 4 commits into from

Conversation

robert-bryson
Copy link
Contributor

Related to GSA/data.gov#4723

Changes proposed in this pull request:

  • Added link preview image to match Catalog

security considerations

[Note the any security considerations here, or make note of why there are none]

@@ -24,13 +24,17 @@
<meta property="og:title" content="{{title}} - Data.gov" />
<meta property="og:description" content="{{excerpt}}" />
<meta property="og:site_name" content="{{ site.title }}"/>
{% assign img = "https://s3-us-gov-west-1.amazonaws.com/cg-0817d6e3-93c4-4de8-8b32-da6919464e61/hero-image-bg.png" %}
Copy link
Member

Choose a reason for hiding this comment

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

When we have a new image in the future, this img will have a broken link. can we add a test on this to ensure the link is valid?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that is a good idea. Ideally when/if we update the image we just replace the old image with a new with the same filename. I do think it would be wise to make sure that it is a valid image, though. Working on a test now.

Copy link
Contributor

Choose a reason for hiding this comment

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

can you test it with this shortcode on your local and see if that works?

// Image exists in S3
{% image "my-image.png" "My PNG Image Alternative Name" false %}

https://github.com/GSA/datagov-11ty/tree/main?tab=readme-ov-file#referencing-images

Copy link
Member

Choose a reason for hiding this comment

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

or better, we can make it a 11ty config item, so that it will always point to the latest url if the image is updated in the 11ty config.

I dont like relying on developer to remember to edit this line whenever there is a image update.
I dont like adding a test either. Even there is a updated image, the old image might not get deleted from S3 therefore the old url is still valid and will pass the test.

Copy link
Contributor

Choose a reason for hiding this comment

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

or better, we can make it a 11ty config item, so that it will always point to the latest url if the image is updated in the 11ty config.

wouldn't we then have to update the config whenever the image is updated? 🌀

Copy link
Contributor

Choose a reason for hiding this comment

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

Working on a test now.

Did you ever get that test finished @robert-bryson ?

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.

3 participants