Skip to content
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

Merged
merged 6 commits into from
Oct 21, 2021

Conversation

nozomuikuta
Copy link
Collaborator

Types of changes

  • Bug fix (a non-breaking change which fixes an issue)
  • New feature (a non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Description

Resolves #831 (thus, #904)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes (if not applicable, please state why)

@nozomuikuta
Copy link
Collaborator Author

By the way, according to Vue documentation, style attribute is also supposed to be set at root level of data object.

Should I fix it as well in this PR or submit another PR?

Copy link
Member

atinux commented Jun 4, 2021

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: {} })
Copy link
Collaborator Author

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>).

Copy link
Member

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Member

@farnabaz farnabaz Jun 10, 2021

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@farnabaz

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. 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@farnabaz

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.

@nozomuikuta nozomuikuta changed the title fix(content): merge css classes fix(content): merge class and style attribute Jun 5, 2021
@ManasMadrecha
Copy link

Any updates?

@atinux
Copy link
Member

atinux commented Jun 22, 2021

We are working with the Docus team to soon release the next version of @nuxt/content with few breaking changes and lot of fixes.

@packet-sent
Copy link
Contributor

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?

Copy link
Member

atinux commented Aug 24, 2021

It's not related to Nuxt 3 development @packet-sent

@nozomuikuta nozomuikuta added the bug Something isn't working label Sep 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vue component classes overriden by end-user specified classes in Markdown page
6 participants