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

WEB: Added SEO functionality #393

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

Conversation

lwcorp
Copy link

@lwcorp lwcorp commented May 11, 2024

Currently ScummVM doesn't use any SEO, meaning if you share pages they all externally preview as if you shared just the homepage, so therefore:

  1. Switched to SEO title
  2. Added SEO meta description
  3. Switched to SEO og:title
  4. Switched to SEO og:description
  5. Switched to SEO og:url
  6. Added SEO canonical URL

This should solve this ticket, but only the site owners can preview this, so please check first it works well for the homepage, articles, compatibility, etc.

I'm auto generating the description based on the same substr principle as in https://github.com/scummvm/scummvm-web/blob/master/templates/components/list_items.tpl

1. Switched to SEO title
2. Added SEO meta description
3. Switched to SEO og:title
4. Switched to SEO og:description
5. Switched to SEO og:url
6. Added SEO canonical URL
@lephilousophe
Copy link
Member

Thank you for your PR.
Your proposal has several drawbacks while rendering the description: the tags are present and the content is not the most appropriate for several pages.
I proposed another approach in #411.

@lwcorp
Copy link
Author

lwcorp commented Sep 6, 2024

Thanks, but while your other approach adds certain features (which mine didn't) it simply ignores others. So why not just edit my PR to merge both of our features?

Otherwise your approach means:

  1. You called your PR "Improve OpenGraph", but then added meta description too, so you should rename your PR accordingly.
  2. Why not fixing <title> too (like I did)?
  3. In meta description, you used if isset($description) so why not add else with my code of $content until the first linebreak?
  4. Likewise for og:description
  5. Likewise for og:title and if isset($subtitle) - why not add else with my code of $content_title?

@lephilousophe
Copy link
Member

Thanks, but while your other approach adds certain features (which mine didn't) it simply ignores others. So why not just edit my PR to merge both of our features?

First, I am not fond of editing others' work behind their back.

Besides, your PR as it is doesn't work at all: there is a syntax error and cutting the line doesn't work for a reason I didn't investigate. I suspect you didn't try it in a development environment.

Finally, as I rewrote a big part of your PR, the only thing remaining is the fix for the URLs: that's not really an iteration over your work anymore. So pushing on your PR would be like a false pretense.

Otherwise your approach means:

1. You called your PR "Improve OpenGraph", but then added meta description too, so you should rename your PR accordingly.

IMHO what is OpenGraph and what is not is not really significant. All of these are web page headers to help other websites to render the page.

2. Why not fixing `<title>` too (like I did)?

When I first tested your PR, I noticed that content_title was not the most adequate as it added redundancy in the browser title (for example in the screenshots page, it says ScummVM :: Screenshots :: Screenshots, and that's not the only page).
I then decided to remove it as it doesn't bring much information to the user.
I changed the way the subtitle work afterwards and I didn't reconsider this change which didn't seem to bring much information. The page title is usually on the title bar and tabs where it is cut shortly. This means that having a long title isn't useful in this case.

After looking at it more, it may help rendering of search results in Bing (interestingly, Google seems to choose a better title than us).

3. In meta description, you used `if isset($description)` so why not add `else` with my code of `$content` until the first linebreak?

4. Likewise for og:description

Because your solution doesn't work where I didn't set a description.
First, it doesn't work like it is intended to as I explained above.
Then, it doesn't strip tags nor escapes them which means that it breaks the page layout when content contains HTML which is... always: content variable is the result of the rendering of another template.
Keeping the first line would only keep a tag.
Finally, on the pages where a description is missing in my PR, there is no valuable text either in the page to display, so your solution can't work either.

5. Likewise for `og:title` and `if isset($subtitle)` - why not add `else` with my code of `$content_title`?

Because content_title is a duplicate in the cases where a subtitle is not set.
Why do you want to add a subtitle on the screenshots global page? There is no need for one and it will only lead to redundancy in the title.

@lwcorp
Copy link
Author

lwcorp commented Sep 7, 2024

After looking at it more, it may help rendering of search results in Bing

Plus you did fix og:title so just apply even your own fix to <title> as well.

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