-
Notifications
You must be signed in to change notification settings - Fork 36
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
DOP-4916: Implement SoftwareSourceCode structured data #1258
DOP-4916: Implement SoftwareSourceCode structured data #1258
Conversation
background-color: ${palette.black}; | ||
color: ${palette.gray.light3}; | ||
.dark-theme & { | ||
border-color: ${palette.gray.dark2}; |
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.
No CSS changes actually applied here
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.
LGTM - left some minor comments below
note - CMIIW but i think there were no styling changes (just shifted from structured data HTML) !
src/components/Video/index.js
Outdated
<script id={`video-object-sd-${url}`} type="application/ld+json"> | ||
{JSON.stringify(structuredData)} | ||
</script> | ||
{videoObjectSd.isValid() && ( |
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.
Matt had suggested doing this in the memo and returning ! 👍
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.
Makes sense. Done. Thanks for flagging (and to Matt for suggesting)
src/utils/get-language.js
Outdated
return undefined; | ||
} | ||
|
||
const fullLangName = LANGUAGE_NAMES[lang]; |
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.
[minor] - can we check for hasOwnProperty here for cases like 'constructor' 😬
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 hope nobody is using constructor
as a programming language. Done
src/components/Code/Code.js
Outdated
@@ -85,77 +86,89 @@ const Code = ({ | |||
reportAnalytics('CodeblockCopied', { code }); | |||
}, [code]); | |||
|
|||
const softwareSourceCodeSd = useMemo(() => new SoftwareSourceCodeSd({ code, lang }), [code, lang]); |
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.
same minor point here for checking validity and returning !
src/components/Code/Output.js
Outdated
<script | ||
type="application/ld+json" | ||
dangerouslySetInnerHTML={{ | ||
__html: softwareSourceCodeSd.toString(), |
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.
Do we need toString() here? Isn't it already stringified above?
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.
Yes! Thank you for catching! Removed!
src/components/Video/index.js
Outdated
<script | ||
type="application/ld+json" | ||
dangerouslySetInnerHTML={{ | ||
__html: videoObjectSd.toString(), |
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.
ditto
@seungpark You are correct! The comment above was just to flag that nothing was actually changed since the diff seemed to suggest variables and stuff were being changed |
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.
Thanks!
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.
LGTM 👍
Stories/Links:
DOP-4916
Current Behavior:
Staging Links:
Google doesn't care about
SoftwareSourceCode
, so they're not present in Rich Results.Notes:
SoftwareSourceCode
structured data for code blocks, withprogrammingLanguage
as optional. A new mapping of different full language names has been added here to help map languages from code blocks to more SEO-friendly names. Since we don't currently have a formal list of supported languages, I'm basing this off of what I've seen and what LeafyGreen supports.Ideally, these would be separate PRs, but I've added a few bonus changes based on related work. Let me know if you'd like me to create separate PRs!
VideoObject
because the Docs Landing SD had it, but it seems like it's not actually used for anything. Less code to worry about!VideoObject
to adopt newStructuredData
class introduced in DOP-4915: Add Tech Article Structured Data to page #1249.BreadcrumbContainer
prop type. This reduces noisy console errors for local dev.README updates