fix($core): preserve original meta charset and viewport (#2332) #2333
Conversation
sakihet
force-pushed
the
fix-meta
branch
2 times, most recently
from
804193a to
b37ccfb
Compare
There was a problem hiding this comment.
Hi @sakihet , thanks for your work!
The fix does preserve chartset and viewport, but as mentioned in #2332 , updateMeta'll remove all meta tags from the template. Like <meta name="generator" ... in the default ssr template.
I'd suggest we extract templateMeta from the template and unionBy them together, what do you think?
|
hi @ludanxer , thanks for suggestion.
looks good. i'm going to fix. |
sakihet
force-pushed
the
fix-meta
branch
2 times, most recently
from
1cef33f to
c27c506
Compare
|
i updated my PR. please review @ludanxer |
| const obj = {} | ||
| obj[attr] = tag.getAttribute(attr) | ||
| templateMeta.push(obj) | ||
| } else if (names.every(e => ['name', 'content'].includes(e))) { |
There was a problem hiding this comment.
wondering about why these if statements are necessary, simply assign each attribute with their name should be fine?
There was a problem hiding this comment.
wondering about why these if statements are necessary, simply assign each attribute with their name should be fine?
There was a problem hiding this comment.
@NeverBehave some meta tags doesn't have name attribute. then, i wrote these if statements.
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/meta
There was a problem hiding this comment.
I am a bit confused, I think the getAttributeNames() will return all names of the attributes from the elements:
> a
<meta charset="utf-8">
> a.getAttributeNames()
Array [ "charset" ]
So basically loop through each of these attributes should be fine? What exactly the name you reference to? If <meta name=""/> then it is also included in the function returns.
> document.querySelectorAll('meta')[1]
<meta http-equiv="X-UA-Compatible" content="IE=Edge">
> document.querySelectorAll('meta')[1].getAttributeNames()
Array [ "http-equiv", "content" ]
> document.querySelectorAll('meta')[1].getAttribute('http-equiv')
"X-UA-Compatible"
The example I provided previous did pass the test tho, correct me if I miss anything
There was a problem hiding this comment.
sorry, i misunderstood. thank you for the information.
| @@ -35,10 +35,26 @@ export default { | |||
|
|
|||
| getMergedMetaTags () { | |||
| const pageMeta = this.$page.frontmatter.meta || [] | |||
| const templateMeta = [] | |||
| if (typeof document !== 'undefined') { | |||
| const templateMetaTags = document.querySelectorAll('meta') | |||
There was a problem hiding this comment.
this.currentMetaTags already gathers meta tag on mount() and reusing it may provide better code readability
There was a problem hiding this comment.
@NeverBehave thanks!
|
Wondering how this pr goes since the related issue really hurts experience on mobile devices. |
|
@sakihet Sorry for the super delay. After some thoughts, change how Change into // init currentMetaTags from pageMeta
this.currentMetaTags = this.getMergedMetaTags().map(m => {
let selector = 'meta'
for (const [key, value] of Object.entries(m)) {
selector += `[${key}="${value}"]`
}
return [...document.querySelectorAll(selector)]
}).flat()What do you think? |
|
@ludanxer no problem.
looks good. thank you for your help. |
|
Hey @ulivz, do you think you can merge this? |
|
...sooo why isn't this merged yet? |
Checkout the references of #2369 : their own websites also affected by this bug and still nobody seems to be worried.
|
Summary
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: