Skip to content

Conversation

pimlie
Copy link
Collaborator

@pimlie pimlie commented Feb 4, 2019

Another issue I noticed by running html-minifier (I am adding an itemscope).

Interestingly, it seems the typeof undefined check will never be true (at least with ssr). I wasnt able to give my attribute an undefined value to only print the attribute tag (edit: not sure where but in my Nuxt project somewhere between nuxt:getNuxtConfig and vue-meta:getMetaInfo the attributes with undefined values are completely stripped).

There are quite a bit of attributes which should never be used on a html/body, but for the sake of completeness I still included them. Maybe in a next major version we could just do a type check on data[attr] === true, but that again could result in invalid html when passing custom attributes so not sure what would be best

@codecov-io
Copy link

codecov-io commented Feb 4, 2019

Codecov Report

Merging #317 into master will decrease coverage by 0.15%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #317      +/-   ##
==========================================
- Coverage   40.89%   40.74%   -0.16%     
==========================================
  Files          19       19              
  Lines         269      270       +1     
==========================================
  Hits          110      110              
- Misses        159      160       +1
Impacted Files Coverage Δ
src/server/generators/attrsGenerator.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 9dfb001...ea28b83. Read the comment docs.

@TheAlexLichter
Copy link
Member

@pimlie Could you add a test for that? ☺️

@pimlie
Copy link
Collaborator Author

pimlie commented Feb 4, 2019

@manniL with the state of the tests in general I'd prefer not to actually. There are no tests yet which validate really the generated html and I dont think its worth my time to add it if vue-meta gets deprecated in its current form because of nuxt/rfcs # 19. Or is that rfc now deprecated since vue-meta is part the nuxt project?

@TheAlexLichter
Copy link
Member

@pimlie I don't think vue-meta nor the RFC will be deprecated. Whether we will use vue-meta to implement that RFC or not isn't clear yet but there are a lot of other projects "depending" on vue-meta ☺️

@TheAlexLichter TheAlexLichter changed the base branch from master to next February 11, 2019 13:33
@TheAlexLichter
Copy link
Member

@pimlie Should I merge that after the refactor? ☺️

@pimlie
Copy link
Collaborator Author

pimlie commented Feb 11, 2019

I think its best to close this one, we will find another way 🚡

@pimlie pimlie closed this Feb 11, 2019
pimlie added a commit to pimlie/vue-meta that referenced this pull request Feb 11, 2019
@pimlie pimlie deleted the fix-boolean-attributes branch March 23, 2019 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants