-
Notifications
You must be signed in to change notification settings - Fork 10
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 not respecting social variables, customized title #176
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/patronage/bubs-next/DTUHoMzcX9UPDtqXzVHqn5pEzjnC |
website/src/components/Meta.js
Outdated
imageUrl.indexOf('http://') === -1 && | ||
imageUrl.indexOf('https://') === -1 | ||
) { | ||
imageUrl = META.url + imageUrl; |
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.
is it possible there's a situation where imageUrl
is relative and without a leading slash and we need to add a slash between META.url
and imageUrl
? Also should we expect users/devs to always set the META.url
without a trailing slash or do we need to check/sanitize that as well?
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.
good feedback, ty. agree we should check/document.
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 note on image url generation, otherwise lgtm
closes #183
Made those change @chrisherold. I realized that the image proxy i implemented was an 80% solution, but missing the actual solve (the issue is that twitter won't index images when you have robots.txt that blocks access, which WP engine does on .wpengine domains). I made a new config variable, and the redirect rule so that next proxies the remote image. Confirmed this is output:
That path then proxies WP, but does so without robot blocking metas that make twitter mad. Also setup our test case to have custom Twitter meta fields and confirmed they work: |
* main: feat: log wp admin users into front-end preview mode (#173)
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
* main: simpler eslint config (no custom babel) (#185) chore: node minor dep upgrades (#188) docs: env variable with graphcdn fix bug in postheader loading wrong scss file fix env.sample domain for graphcdn fix: meta not respecting social variables, customized title (#176) feat: log wp admin users into front-end preview mode (#173) fix: Get stylelint working again (#182) feat: Next 12 upgrade (#180) feat: bumping husky to version 7 (#179) resolve bug where clicking 'view' in wp-admin leads to a broken redirect (#165) feat: new convenience plugins in wordpress admin (#166) feat: theme mod instead of acf options for preview mode url (#163)
refs #174
refs #172
refs #177