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

Added support for preview links #2184 #2187

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

seagulley
Copy link

The PR fulfills these requirements: (check all the apply)

  • It's submitted to the main branch.
  • When resolving a specific issue, it's referenced in the PR's title (e.g. feat: Add a button #xxx, where "xxx" is the issue number).
  • When resolving a specific issue, the PR description includes Closes #xxx, where "xxx" is the issue number.
  • If changes were made to ui folder, unit tests (make test) still pass.
  • New/updated tests are included

Added new variables to conf.go ServerConf: PreviewImage, PreviewTitle, PreviewDescription.
Modified web_server.go to use these new values as meta tags in the header of the root index.html.

Closes #2184

Copy link
Collaborator

@mturoci mturoci left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @seagulley! I would like to see some demo to test these changes in action.

Comment on lines +55 to +57
PreviewImage string
PreviewTitle string
PreviewDescription string
Copy link
Collaborator

Choose a reason for hiding this comment

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

Make these part of the https://github.com/h2oai/wave/blob/94bf37f538719ad279a41b0df1b452623d61aaa4/conf.go#L71C7-L71C7 as well to expose new options to the users.

Also document here.

Copy link
Author

Choose a reason for hiding this comment

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

I'll get started on this as well as a demo :)

Copy link
Author

Choose a reason for hiding this comment

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

Hi Martin,

I started working on the demo. Using Meta's Open Graph debugger, it can read the custom title and description, but there are some issues with the image.
image
image

I did some more digging and there are some guidelines for adding images to preview links
https://developers.facebook.com/docs/sharing/best-practices/#precaching

How should I approach this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The warning says all it needs are og:image:height and og:image:width props. I would first try it out to see if they help. If they do, the next step would be to validate passed image dimensions and:

  • If the image does not adhere to the recommended ratio or is too small, log warning and use the actual dimensions for the respective props.
  • If the image ratio is good and >= required dimensions, use the copy of the image resized to the expected dimensions.

Hope that makes sense.

Copy link
Author

Choose a reason for hiding this comment

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

Yes that makes sense I'll get started on it!

web_server.go Show resolved Hide resolved
if auth != nil {
fs = auth.wrap(fs)
}
return &WebServer{site, broker, fs, keychain, maxRequestSize, baseURL}, nil
}

func mungeIndexPage(baseURL, html string) string {
func mungeIndexPage(baseURL, html string, previewImage string, previewTitle string, previewDescription string) string {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Should be enough.

Suggested change
func mungeIndexPage(baseURL, html string, previewImage string, previewTitle string, previewDescription string) string {
func mungeIndexPage(baseURL, html, previewImage, previewTitle, previewDescription string) string {

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.

Add support for preview links for public sharing
2 participants