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

add customClass props to Skeletons #3642

Closed

Conversation

GMartigny
Copy link
Contributor

Proposed Changes

  • Add a customClass prop to Skeletons
  • Use <template> rendering instead of functionnal

@jtommy
Copy link
Member

jtommy commented Jan 21, 2022

@GMartigny why did you move to template rendering?

@GMartigny
Copy link
Contributor Author

Because functional didn't serve any purpose here and all the rest of the lib is template. But of course, it can be reverted if you think it's out of scope for this PR.

@jtommy
Copy link
Member

jtommy commented Jan 22, 2022

And custom-class? You can use class and it will be applied to root element.
Probably with functional is not possible, but you can add reading it from vnode (second parameter of render function), for example: vnode.data.staticClass

Functional are great in Vue 2 for a component without state and there are benchmarks in the web that show rendering improvements.

@GMartigny
Copy link
Contributor Author

GMartigny commented Jan 22, 2022

Probably with functional is not possible, but you can add reading it from vnode (second parameter of render function), for example: vnode.data.staticClass

This is indeed need to be explicit with functional rendering.

Functional are great in Vue 2 for a component without state and there are benchmarks in the web that show rendering improvements.

Didn't know, thanks for the infos. However, I still find it quite hard to parse for a human.

I took the liberty to spread the whole data coming from the context (...context.data) into the component. This allow users to also bind id and event listeners. What do you think ?

@jtommy
Copy link
Member

jtommy commented Jan 22, 2022

Ok!

staticClass: 'b-skeleton',
class: [ context.props.size, context.props.position, { 'is-animated': context.props.animated } ]
class: [ context.data.attrs.class, context.props.size, context.props.position, { 'is-animated': context.props.animated } ]
Copy link
Member

Choose a reason for hiding this comment

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

Are you sur is it ok? I usually use .staticClass and not .class

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you know the proper way to test this ?

If I do :

wrapper = mount(BSkeleton, {
  attrs: {
    class: 'custom-class'
  }
})

I don't have staticClass set.

Copy link
Member

@jtommy jtommy Jan 27, 2022

Choose a reason for hiding this comment

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

I mean in the core code, you should use context.data.attrs.statiClass instead of context.data.attrs.class

@QuentinGab
Copy link

Hi !

Is there any update on this @GMartigny ? It would be cool to be able to pass classes and styles to the component

@GMartigny
Copy link
Contributor Author

Honestly, I don't know how to do it properly in the functional way (especially with testing). Maybe @jtommy or someone else could finish this up.

@jtommy
Copy link
Member

jtommy commented Mar 13, 2022

As i told you https://github.com/buefy/buefy/pull/3642/files#r794054499, did you try it?

@jtommy
Copy link
Member

jtommy commented Sep 13, 2022

@GMartigny any news?

@GMartigny
Copy link
Contributor Author

As I said in my previous comment, I'm not familiar with Vue functionnal rendering. I tried your suggestion, but I'm not able to properly test it. If i use context.data.attrs.staticClass I don't have the class correctly set in tests.

@wesdevpro
Copy link
Member

This PR has been migrated to a feature request discussion. Please see #3976 for reference.

@wesdevpro wesdevpro closed this Jan 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants