Skip to content
This repository was archived by the owner on Oct 1, 2025. It is now read-only.

Conversation

clarkdo
Copy link
Member

@clarkdo clarkdo commented Mar 19, 2019

@clarkdo clarkdo requested review from atinux and pi0 March 19, 2019 16:23
@codecov-io
Copy link

codecov-io commented Mar 19, 2019

Codecov Report

Merging #334 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #334   +/-   ##
=======================================
  Coverage   40.89%   40.89%           
=======================================
  Files          19       19           
  Lines         269      269           
=======================================
  Hits          110      110           
  Misses        159      159
Impacted Files Coverage Δ
src/server/generators/titleGenerator.js 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2742965...3aea471. Read the comment docs.

return {
text () {
return `<${type} ${attribute}="true">${data}</${type}>`
return data ? `<${type} ${attribute}="true">${data}</${type}>` : ''
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't think of this behavior normally (when using vue-meta in context of a typical SPA and not going for microfrontends or similar). Please add at least a comment there ☺️

Also, maybe checking for undefined instead of using all falsy values might make sense? 🤔

Copy link
Member Author

@clarkdo clarkdo Mar 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the checking to String(data) for keeping it same behaviour as document.title=....

document.title = undefined // <title>undefined</title>
document.title = null // <title>null</title>
document.title = false // <title>false</title>
document.title = 0 // <title>0</title>
document.title = '' // <title> tag will be removed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes sense to be in line with document.title here but I'm not sure if being in line with the vue-meta-own conventions might be more important (though that will only be relevant for v2)

@TheAlexLichter TheAlexLichter changed the title fix: not generate title tag if title is empty fix: don't generate title tag if title is empty Mar 19, 2019
@TheAlexLichter
Copy link
Member

If that PR gets merged, we have to port it to the nextranch too ☺️

@TheAlexLichter TheAlexLichter requested a review from pimlie March 19, 2019 16:33
@pimlie
Copy link
Collaborator

pimlie commented Mar 19, 2019

I am ok with this as html5 spec says the title element is required unless its reasonable not to. Also chrome/firefox behave the same when title doesnt exists as when its an empty string.

Even though you could also argue this issue should be fixed in Nuxt, because vue-meta already provides the option to ignore the title on ssr by simply not calling {{ $meta.title.text() }} in your template.

@clarkdo
Copy link
Member Author

clarkdo commented Mar 19, 2019

@pimlie I fixed this issue in vue-meta since nuxt needs to resolve global and component head title config for checking if meta.title should be called which may bring unnecessary code and overhead.

@pimlie
Copy link
Collaborator

pimlie commented Mar 19, 2019

@clarkdo the other options would be to either pass an argument to text() to make the behaviour optional (less favourable) or add a global config option so users can indicate that a vue-meta property should be ignored (regardless whether it has a value or not).

Strictly speaking (as in according to the spec) the title element could not only be not printed on empty string but also when it only contains inter-element white space. So String(data).trim() would maybe be more in-line with the spec. The end result in the browser will be exactly the same though as the browser will ignore a title containing only spaces/line-breaks anyways.

@atinux
Copy link
Member

atinux commented Mar 28, 2019

Ready to merge for minor, I let you handle it @pimlie when you feel ready for it :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants