-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
b-button: Remove warning by importing b-link #201
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
Conversation
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.
Thanks :) Just left some comments if you can fix them too.
@@ -18,7 +20,7 @@ | |||
]; | |||
}, | |||
componentType() { | |||
return (this.href || this.to) ? 'b-link' : 'button'; | |||
return (this.href || this.to) ? bLink : 'button'; |
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 think you have to keep this as is (b-link) as kebab case is preferred.
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 not a string. It's a variable name.
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 didn't take time to explain the reasoning as it's the same as #174
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.
@gpyh,
comparing to #174, this piece specially omitted or forgotten?
components:{
bLink
},
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 actually doesn't need the component to be registered as it uses directly its description.
It's still a good idea to add it though. I'll do that.
It used to embed the b-link component without importing it
Is something still blocking this PR? I realize I didn't motivate the change properly. Allow me to fix this: I use bootstrap-vue by importing each component manually instead of registering them in the global scope. It makes the final bundle smaller and the code more explicit. Say I have a component A in my app that depends on a bootstrap-vue component called B, which itself depends on another bootstrap-vue component named C. The dependency graph looks roughly like this:
The code for A would be roughly this:
My app has no way of knowing that B depends on C. It can only work with the tag name. At this point that tag name is simply a string unrelated to any actual component. Registering this transitive dependency in my components would look like this:
With this I can use C in A but the C in B still doesn't render. It's because the components are registered locally ; Vue doesn't walk up the components hierarchy. A component is either global or strictly local. To fix this, bootstrap-vue needs to be explicit in the dependency it uses. It means importing and registering the appropriate components. For #174 this was what I did. Here, the component is dynamically determined. I only made those fixes as the rendering errors appeared in my code, but in my opinion a better course of action would be to enforce this pattern across every component. I can carry out that task of reviewing all the components and making this fix if you want me too. |
@gpyh Thanks for your explanations and we totally agree about this to be done in entire components, The only question I had was that why we should point to reference instead of string |
Long time since it is open. Merging this as a starting point. Will discuss before migration other components. |
Same drill as last time. (#174)