Skip to content

Add base URI to meta image URLs#9125

Merged
ahukkanen merged 3 commits intodecidim:developfrom
mainio:fix/meta_image_base_url
Apr 4, 2022
Merged

Add base URI to meta image URLs#9125
ahukkanen merged 3 commits intodecidim:developfrom
mainio:fix/meta_image_base_url

Conversation

@lahdeero
Copy link
Copy Markdown
Contributor

@lahdeero lahdeero commented Apr 1, 2022

🎩 What? Why?

When linking Decidim instance to social media image is usually broken (see screenshot below) because it doesn't include base url (e.g. https://meta.decidim.org). Correct usage of social meta tags is described for example in Twitter docs.

Testing

  1. Go to any decidim instance (e.g. https://meta.decidim.org/)
  2. Open console (F12 in Firefox)
  3. Type these commands
document.querySelector("meta[name='twitter:image']").content
document.querySelector("meta[property='og:image']").content
  1. Content should be like https://meta.decidim.org/rails/active_storage/blobs/123--foobar.png

📋 Checklist

🚨 Please review the guidelines for contributing to this repository.

  • CONSIDER adding a unit test if your PR resolves an issue.
  • ✔️ DO check open PR's to avoid duplicates.
  • ✔️ DO keep pull requests small so they can be easily reviewed.
  • ✔️ DO build locally before pushing.
  • ✔️ DO make sure tests pass.
  • ✔️ DO make sure any new changes are documented in docs/.
  • ✔️ DO add and modify seeds if necessary.
  • ✔️ DO add CHANGELOG upgrade notes if required.
  • ✔️ DO add to GraphQL API if there are new public fields.
  • ✔️ DO add link to MetaDecidim if it's a new feature.
  • AVOID breaking the continuous integration build.
  • AVOID making significant changes to the overall architecture.

📷 Screenshots

image
image

♥️ Thank you!

@andreslucena andreslucena added module: core type: fix PRs that implement a fix for a bug labels Apr 4, 2022
Copy link
Copy Markdown
Member

@andreslucena andreslucena left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the PR

@andreslucena
Copy link
Copy Markdown
Member

There's an unrelated failing spec, already reported at #9017
I'll rerun the CI.

Copy link
Copy Markdown
Contributor

@ahukkanen ahukkanen left a comment

Choose a reason for hiding this comment

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

👍

@ahukkanen ahukkanen merged commit 0b6e0de into decidim:develop Apr 4, 2022
@ahukkanen ahukkanen deleted the fix/meta_image_base_url branch April 4, 2022 13:36
entantoencuanto added a commit to PopulateTools/decidim that referenced this pull request Apr 5, 2022
* chore/meetings_optimization:
  Add touch:true to Follow association with followable
  Fragment cache meetings partial in meetings index
  Bump minimist and node-forge (decidim#9131)
  Bump puma from 5.6.2 to 5.6.4 (decidim#9132)
  Add base URI to meta image URLs (decidim#9125)
  Make Decidim fully configurable via ENV vars part II (decidim#8990)
  Allow assembly admins to manage components in child assemblies (decidim#8955)
  Export calendar improvements (decidim#9035)
  Add reminders for publishing reports to meeting authors (decidim#8757)
  VAPID key generator availabe in core (decidim#9107)
  Dont add external link container inside editor (decidim#9095)
@ahukkanen
Copy link
Copy Markdown
Contributor

Can we also backport this to 0.26 please?

lahdeero pushed a commit to mainio/decidim that referenced this pull request Apr 8, 2022
* Add base url to meta image urls

* No need to have organization in every test

* Use variable iinstead repeating
ahukkanen pushed a commit that referenced this pull request Apr 12, 2022
* Add base url to meta image urls

* No need to have organization in every test

* Use variable iinstead repeating
@PierreMesure
Copy link
Copy Markdown
Contributor

Great! I wonder if the methods you added could be built on to implement this improvement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

module: core type: fix PRs that implement a fix for a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants