-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
core(artifacts): add property attribute to MetaElements #9978
core(artifacts): add property attribute to MetaElements #9978
Conversation
looks good, thanks! could you also add a meta element to one of our smoke tests ( |
@connorjclark Thanks for the tip, fixed it! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one last thing then we can merge. thanks for the pr!
@@ -25,6 +25,7 @@ class MetaElements extends Gatherer { | |||
return { | |||
name: meta.name.toLowerCase(), | |||
content: meta.content, | |||
property: meta.attributes.property ? meta.attributes.property.value : '', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be undefined
instead of ''
when not found
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was in doubt about this, because content
is ''
, also if it's not defined. So I kept it in line. But since property
isn't in the HTML5 spec, I understand your comment as well.
Summary
The MetaElements artifacts contains now also de meta element attribute
property
. Now only thename
attribute was present. Theproperty
attribute is often use by Open Graph meta tags. More information aboutproperty
: https://stackoverflow.com/questions/22350105/whats-the-difference-between-meta-name-and-meta-propertyRelated Issues/PRs
Fixes #9964