-
-
Notifications
You must be signed in to change notification settings - Fork 626
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(content): merge class and style attribute #905
Conversation
By the way, according to Vue documentation, Should I fix it as well in this PR or submit another PR? |
I think you can fix it in this PR too |
@@ -53,7 +59,7 @@ function propsToData (node, doc) { | |||
obj[attribute] = value | |||
} | |||
return data | |||
}, { attrs: {} }) | |||
}, { class: null, style: null, attrs: {} }) |
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.
It is good to set initial value null
, for Vue not to render "empty" css class (e.g. <p class=""></p>
).
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.
This is strange, Vue should not create empty class
attribute when there is no value for it.
Do you mind to create a simple reproduction for this issue?
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.
Here is a demo, and here is the related part in the official doc.
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.
Thank you for the demo, but I mean a reproduction with nuxt-content.
I agree that having class: ''
cause creating and empty attribute.
Having null as default value for class and style is not necessary, and it should work without these defaults.
I suggest to remove these defaults and create another PR/issue with a reproduction sample that point the issue with nuxt-content. The reproduction may show how adding these default could solve the existing issue.
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 suggest to remove these defaults and create another PR/issue with a reproduction sample that point the issue with nuxt-content. The reproduction may show how adding these default could solve the existing issue.
I pushed another commit to remove default values of class
and style
.
By the way, I think we don't need another PR to consider default values, because my reproduction is enough to demonstrate Vue's behavior, and Nuxt Content doesn't add its specific behavior. It is just simply a Vue functional component.
I believe this PR is now ready to merge. 🤔
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 do appreciate if you take a look at my last comment and commit. 🙏
I pushed another commit to remove default values of class and style.
Any updates? |
We are working with the Docus team to soon release the next version of @nuxt/content with few breaking changes and lot of fixes. |
Is the update going to happen after Nuxt 3 becomes a public beta? |
It's not related to Nuxt 3 development @packet-sent |
Types of changes
Description
Resolves #831 (thus, #904)
Checklist: