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

Fix meta image #1647

Merged
merged 2 commits into from
Mar 25, 2021
Merged

Fix meta image #1647

merged 2 commits into from
Mar 25, 2021

Conversation

amyrlam
Copy link
Member

@amyrlam amyrlam commented Mar 20, 2021

Resolves: #1621

Related to: ember-learn/ember-website#804

@amyrlam amyrlam mentioned this pull request Mar 20, 2021
@amyrlam
Copy link
Member Author

amyrlam commented Mar 20, 2021

I copied the contents from https://github.com/ember-learn/ember-website/pull/804/files except for <meta name="google-site-verification" as wasn't sure what that was yet.

ijlee2
ijlee2 previously requested changes Mar 22, 2021
Copy link
Member

@ijlee2 ijlee2 left a comment

Choose a reason for hiding this comment

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

@amyrlam Thanks for working on a solution. I checked that the image shows up when I post a Guides link in a tweet.

Before we merge the pull request, I'd like to see application tests added so that there's no regression in the future. Can we update the file https://github.com/ember-learn/guides-source/blob/master/tests/acceptance/meta-data-test.js in the following manner:

test('We set title and description correctly', async function(assert) {
  // Navigate to Routing, Defining Your Routes

  // Check title and description

  // Navigate to Services, Overview

  // Check title and description
});

test('We set metadata for Open Graph correctly', async function(assert) {
  // Navigate to Routing, Defining Your Routes

  // Check title, description, and image

  // Navigate to Services, Overview

  // Check title, description, and image
});

test('We set metadata for Twitter correctly', async function(assert) {
  // Navigate to Routing, Defining Your Routes

  // Check title, description, and image

  // Navigate to Services, Overview

  // Check title, description, and image
});

You can use QUnit DOM to make assertions against elements in <head>. For example,

assert
  .dom(document.querySelector('meta[name="twitter:title"]'))
  .hasAttribute(
    'content',
    'Defining Your Routes - Routing - Ember Guides',
    'We set the title correctly.'
  );

assert
  .dom(document.querySelector('meta[name="twitter:description"]'))
  .hasAttribute(
    'content',
    /When your application starts, the router matches the current URL/,
    'We set the description correctly.'
  );

assert
  .dom(document.querySelector('meta[name="twitter:image"]'))
  .hasAttribute(
    'content',
    'https://blog.emberjs.com/images/logos/e-icon.png',
    'We set the image correctly.'
  );

app/index.html Outdated
@@ -3,7 +3,13 @@
<head>
<meta charset="utf-8">
<meta http-equiv="X-UA-Compatible" content="IE=edge">
<meta name="description" content="Ember.js helps developers be more productive out of the box. Designed with developer ergonomics in mind, its friendly APIs help you get your job done—fast.">
Copy link
Contributor

Choose a reason for hiding this comment

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

This is applied by guidemaker so it shouldn't be hardcoded in here.

@amyrlam
Copy link
Member Author

amyrlam commented Mar 25, 2021

From the Learning Team meeting: This tests should be made in guidemaker, I just made a separate issue: empress/guidemaker#51

@locks
Copy link
Contributor

locks commented Mar 25, 2021

We reviewed this at the learning meeting and decided to move the PR forward after removing the description since it is applied by guidemaker.
There are tests in guidemaker and empress for meta tags so we felt like upstreaming to those projects made more sense than adding the tests here.
We will follow with further upstream work.

@locks locks enabled auto-merge March 25, 2021 18:04
@locks locks dismissed ijlee2’s stale review March 25, 2021 18:07

learning team decided to upstream work a posteriori

@locks locks self-requested a review March 25, 2021 18:07
@locks locks merged commit c443f66 into master Mar 25, 2021
@locks locks deleted the bug/fix-meta-image branch March 25, 2021 18:07
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 back meta image
3 participants