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

FI-2622: Update meta tags for link unfurling #553

Merged
merged 13 commits into from
Nov 29, 2024
Merged

Conversation

AlyssaWang
Copy link
Collaborator

Summary

Adds custom meta tag handling for URL unfurling. There is a follow-on to investigate the usage of React Helmet to replace dynamic title generation.

Testing Guidance

Install the Google Chrome extension META SEO Inspector (https://chromewebstore.google.com/detail/meta-seo-inspector/ibkclpciafdglkjkcibmohobjkcfkaef) and check the meta tags for title and description. Once merged and pushed to prod, this should also be tested against https://www.zelolab.com/free-tools/preview-as/ for other social media unfurls.

@AlyssaWang AlyssaWang requested a review from arscan October 30, 2024 18:17
Copy link

codecov bot commented Oct 30, 2024

Codecov Report

Attention: Patch coverage is 79.24528% with 11 lines in your changes missing coverage. Please review.

Project coverage is 83.93%. Comparing base (e2773e9) to head (5bc9b78).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...nt/src/components/TestSuite/TestSessionWrapper.tsx 45.45% 6 Missing ⚠️
...c/components/SuiteOptionsPage/SuiteOptionsPage.tsx 54.54% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #553      +/-   ##
==========================================
- Coverage   83.97%   83.93%   -0.04%     
==========================================
  Files         262      263       +1     
  Lines       11430    11459      +29     
  Branches     1260     1261       +1     
==========================================
+ Hits         9598     9618      +20     
- Misses       1822     1831       +9     
  Partials       10       10              
Flag Coverage Δ
backend 92.17% <100.00%> (+<0.01%) ⬆️
frontend 79.15% <78.84%> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

<meta name="og:type" content="website" />
<meta name="og:site_name" content="Inferno" />
<meta name="twitter:card" content="summary" />

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm really curious if sites will load JavaScript up, render it, and then look at the meta tags... or just look at the html of the original page. If it just looks at html and doesn't try the Javascript, I think just adding the inferno logo image (no text) here and 'Inferno Test Session' for the title would be helpful.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For some sites, html will just be pulled -- I think this is a bit unavoidable unless we do some backend handling.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alright, I think this is worth a go to see how it looks in practice for the common sharing spots. If you have time and interest you could check out simplifying this and making it more robust via backend html rendering, but if you have neither and this works I'm very much fine with this approach.

Copy link
Contributor

@arscan arscan left a comment

Choose a reason for hiding this comment

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

I tried out the instructions and could see the metadata alright. I'm a bit skeptical that this will work in practice because it involves having the bot that grabs this to fully render JavaScript and that seems like a big ask. Having a bit more of a fallback in index.html.erb seems like a decent idea (adding an image and just say 'Inferno Test Session').

I wonder if the right solution would have been to just update how index.html.erb is handled such that it gets suite and / or session info via a controller and dumps that in the header that is rendered html via the erb here and here? Not sure exactly what would need to happen on the back end to make that work but it certainly would simplify the javascript and be bullet-proof for clients.

If we want to merge this in and just try it out, I don't see any harm though.

@AlyssaWang
Copy link
Collaborator Author

The problem with adding more fallback in the erb is that it won't be properly overwritten by React meta tags if they have the same identifier, I can investigate further here. Ideally yes, the backend would be handing everything over but I also don't have a good grasp of what would need to happen to make that feasible.

@AlyssaWang
Copy link
Collaborator Author

Ok I've made a couple updates to how the meta tags work:

  1. Added some more tags to index.html.erb so those tags will always populate
  2. Added a MetaTags component so title and description can be set on multiple pages with some consistency

This should cover most cases for link unfurling. The one exception is if we want to overwrite some of the tags, e.g. description, then we will have to use a 3rd party library or implement SSR (which is way too complicated of a solution for this).

@AlyssaWang AlyssaWang requested a review from arscan November 29, 2024 18:24
Copy link
Contributor

@arscan arscan left a comment

Choose a reason for hiding this comment

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

I think this is a good iteration and we'll see if in practice the spots inferno sessions get shared need any stylistic tweaks or needs to be switched to rendering via html on the backend.

@AlyssaWang AlyssaWang merged commit 9d0288e into main Nov 29, 2024
10 checks passed
@AlyssaWang AlyssaWang deleted the FI-2622-social-media branch November 29, 2024 20:52
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.

2 participants