-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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: duplicate meta tags #2164
fix: duplicate meta tags #2164
Conversation
Hi @ludanxer, Thanks for the PR. Here you're just removing the current page meta tags 😅 But what we would like is replacing the global meta tags with the current page meta tags if page meta tags exists. The solution would be a merge, with a priority on page meta tags. |
@kefranabg Thanks for your review and sorry for the delay. I think I'm just removing one set of page meta tags. I understand how things look like, so I'll try to explain what I'm trying to do. Head meta tagsHead meta tags are inserted differently in dev
build
Page meta tagsPage meta tags are managed by updateMeta.js in both devbuildBoth
and updateMeta.js which is causing the duplicate. |
I just made a test with your branch, and global meta tag are always taking priority on page meta tag. It should be the opposite 😄 If global meta tags and page meta tag are defined, page meta tag have priority |
Need some time to fix that. Thanks! |
I also found this problem. These problems (#112, #565, #665, #2133) two years ago seem to be finally fixed. <meta name = "twitter: image" content = "a.png">
<meta name = "twitter: image" content = "b.png"> The above example will result in two pictures for twitter sharing
|
I made a test branch and deploy it. Hope it helps. And I noticed watch: {
$page () {
this.updateMeta()
}
} Keeps getting triggered when I scroll past headers in a page |
Thank you for your efforts |
Hey, I made some tests, if I add a the meta tag For example:
Let me know if you want more context |
This reverts commit 5a02e2a.
I think it's ready now 🤔Thanks for the patience ❤️ |
@ludanxer Sorry for the delay, I'll do a review ASAP 😉 |
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 just added some suggestions and ask some questions. Please check everything is still working as expected if you apply changes 👍
Co-Authored-By: Franck Abgrall <[email protected]>
Previous method will init siteMetaTags with entry page meta tags instead of site meta tags
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, one more review and we'll get that merge 👍 thanks @ludanxer
return meta.map(m => { | ||
let res = `<meta` | ||
Object.keys(m).forEach(key => { | ||
res += ` ${key}="${m[key]}"` |
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.
Shouldn't this be escaped? For example, this config.js
:
module.exports = {
description: 'Image cache & resize service',
}
renders as (within version 1.4.1):
<meta name="description" content="Image cache & resize service">
which is probably not correct. fwiw, version 1.4.0 didn't had this problem.
Suggestion:
res += ` ${key}="${m[key]}"` | |
res += ` ${key}="${escape(m[key])}"` |
and this import at the top of the file:
const escape = require('escape-html')
* fix: duplicate meta in ssr * fix: page metas have higher priority * Revert "fix: duplicate meta in ssr" This reverts commit 5a02e2a. * fix: render meta tags during ssr * improve readability from suggestions Co-Authored-By: Franck Abgrall <[email protected]> * fix: missing spaces * refactor: remove unnecessary code * fix: siteMetaTags aren't correctly init Previous method will init siteMetaTags with entry page meta tags instead of site meta tags Co-authored-by: Franck Abgrall <[email protected]>
Summary
leave meta tags updating to updateMeta.js
fix #777 , relate to #2153
What kind of change does this PR introduce? (check at least one)
If changing the UI of default theme, please provide the before/after screenshot:
Does this PR introduce a breaking change? (check one)
If yes, please describe the impact and migration path for existing applications:
The PR fulfills these requirements:
fix #xxx[,#xxx]
, where "xxx" is the issue number)You have tested in the following browsers: (Providing a detailed version will be better.)
If adding a new feature, the PR's description includes:
To avoid wasting your time, it's best to open a feature request issue first and wait for approval before working on it.
Other information:
I'm not sure what to do when page meta is conflict with site meta, for example
merge them or use page meta to override site meta like
description