-
-
Notifications
You must be signed in to change notification settings - Fork 138
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
feat:add custom twitter_card selection #486
base: main
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughThe changes in this pull request involve formatting and whitespace adjustments in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant EJS Template
participant OpenGraph
User->>EJS Template: Request page
EJS Template->>OpenGraph: Check twitter_card
OpenGraph-->>EJS Template: Return twitter_card value
EJS Template->>User: Render page with Open Graph meta tags
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
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.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
layout/components/header/head.ejs (1)
Line range hint
29-34
: Consider enhancing the deprecation messageWhile the current deprecation message is helpful, consider adding information about the new twitter_card customization feature in the example configuration:
checkDeprecation( theme.global.open_graph === true, 'open_graph', - "Using 'open_graph: true' is deprecated. Please update to the new format:\nglobal:\n open_graph:\n enable: true\n image: your_image_path\n description: your_description" + "Using 'open_graph: true' is deprecated. Please update to the new format:\nglobal:\n open_graph:\n enable: true\n image: your_image_path\n description: your_description\n twitter_card: summary_large_image # Customize twitter card type" )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
_config.yml
(12 hunks)layout/components/header/head.ejs
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- _config.yml
🔇 Additional comments (1)
layout/components/header/head.ejs (1)
44-44
: LGTM: Twitter card customization implementation
The implementation properly handles the twitter card customization with a clear fallback chain:
- Page-specific setting (
page.twitter_card
) - Theme-level setting (
theme.global.open_graph?.twitter_card
) - Default value (
'summary'
)
Also applies to: 50-50
let twitterCard = page.twitter_card || theme.global.open_graph?.twitter_card || 'summary'; | ||
%> | ||
<%- open_graph({ | ||
image: ogImage, | ||
author: author, | ||
description: ogDescription, | ||
twitter_card: 'summary', | ||
twitter_card: twitterCard, | ||
twitter_image: ogImage, | ||
}) %> |
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.
🛠️ Refactor suggestion
Add validation for twitter_card values
Twitter only allows specific values for the twitter:card meta tag: 'summary', 'summary_large_image', 'app', or 'player'. Consider adding validation to ensure only valid values are used.
- let twitterCard = page.twitter_card || theme.global.open_graph?.twitter_card || 'summary';
+ const VALID_TWITTER_CARDS = ['summary', 'summary_large_image', 'app', 'player'];
+ let twitterCard = page.twitter_card || theme.global.open_graph?.twitter_card || 'summary';
+ if (!VALID_TWITTER_CARDS.includes(twitterCard)) {
+ console.warn(`Invalid twitter_card value: ${twitterCard}. Falling back to 'summary'`);
+ twitterCard = 'summary';
+ }
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
let twitterCard = page.twitter_card || theme.global.open_graph?.twitter_card || 'summary'; | |
%> | |
<%- open_graph({ | |
image: ogImage, | |
author: author, | |
description: ogDescription, | |
twitter_card: 'summary', | |
twitter_card: twitterCard, | |
twitter_image: ogImage, | |
}) %> | |
const VALID_TWITTER_CARDS = ['summary', 'summary_large_image', 'app', 'player']; | |
let twitterCard = page.twitter_card || theme.global.open_graph?.twitter_card || 'summary'; | |
if (!VALID_TWITTER_CARDS.includes(twitterCard)) { | |
console.warn(`Invalid twitter_card value: ${twitterCard}. Falling back to 'summary'`); | |
twitterCard = 'summary'; | |
} | |
%> | |
<%- open_graph({ | |
image: ogImage, | |
author: author, | |
description: ogDescription, | |
twitter_card: twitterCard, | |
twitter_image: ogImage, | |
}) %> |
💡 Codebase verification
Documentation needed for Twitter Card configuration
The feature is implemented and configured in _config.yml
with a helpful comment showing valid options (summary/summary_large_image/app/player
), but lacks proper documentation. The configuration exists in:
- Default configuration in
_config.yml
- Implementation in
layout/components/header/head.ejs
- Support for both page-level and global theme configuration
Please add documentation that covers:
- Available Twitter Card types (
summary
,summary_large_image
,app
,player
) - How to configure at theme level in
_config.yml
- How to override at page level using front matter
🔗 Analysis chain
Verify documentation and usage
Please ensure that this new feature is properly documented and examples are provided in the theme's documentation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the feature is documented
echo "Checking documentation for twitter_card feature..."
rg -i "twitter.?card" docs/ README.md
# Check for existing usage in theme files
echo "Checking current usage in theme files..."
rg "twitter_card:" _config.yml
Length of output: 385
Script:
#!/bin/bash
# Let's check for any existing documentation in other common locations
echo "Checking for documentation in other locations..."
fd -t f -e md . | xargs rg -i "twitter.?card"
# Check theme configuration schema if it exists
echo "Checking theme configuration schema..."
fd "schema" -t f -e json -e yaml | xargs rg -i "twitter.?card"
# Check for any examples or templates
echo "Checking example configurations..."
fd "_config" -t f | xargs rg -i "twitter.?card"
Length of output: 783
I found that every page' s twitter_card attribute is 'summary', so I make it Customizable。
The code changes are similar to the PR I submitted last time
Summary by CodeRabbit
New Features
twitter_card
property in Open Graph meta tags.twitter_card
value.Bug Fixes
open_graph
property to guide users towards the new format.Style
_config.yml
file.