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
Bug: event name casing is not standard #310
Comments
Hey @stokaboka, good catch I'll switch this to a bug report To resolve, we should:
Should be a super easy PR if someone is looking to get their feet wet! |
I just started using VueCarousel in a personal project and really love it - thanks very much for the great work! I was reading the issues and had a quick dive into this one. I see that @stokaboka already submitted a merged PR to change With regard to your second point, I did an audit on the code and found three other events that are still formatted in camelCase, namely:
So these would have to be changed as well. Vue's Event Names documentation recommends that events should be in kebab-case; i.e:
But this is inconsistent with the other existing lowercased events Therefore I guess it might be more prudent to change events in camelCase events to lowercase:
Let me know if that's your preference and I can submit a PR! |
As an aside, I think some of the events are not documented, but I haven't checked if someone else has opened another issue or PR for this. |
Hey @darraghenright, thanks for taking on the issue! I would be fine with non-hyphenated ( Let me know if you have any questions |
Yeah, I'd be happy to migrate all events - definitely seems like a good idea to adhere to the Vue style guide there! In terms of backward compatibility, do you mean we should leave any existing events as-is, and add the new events, in case there is userland code that depends on the old events? If so, taking an example in Carousel.vue, we'd do something like: currentPage(val) {
this.$emit("pageChange", val);
this.$emit("page-change", val);
this.$emit("input", val);
}, With a view to removing the |
Speaking of backwards compatibility - I was re-reading Vue's event naming recommendations, and there was an implication that I didn't pick up previously: event listeners are case insensitive in DOM templates, which of course means it's not possible to listen to emitted events in However, in string templates, event listeners are case-sensitive. So this could mean that there are users out there successfully listening for emitted events Just wondering now, if there would be unitended consequences in changing the existing |
@darraghenright yes thats what I was suggesting by backwards compatibility! Feel free to tweak it or clean up the code as you wish - maybe computing the value could make it cleaner? If not I'm good with the solution above. I'll issue a deprecation notice with your works release and we can switch to entirely vue-standard events on the next major. As for the DOM templates, I think it's fine as long as we support both camel and lower case, and deprecate the former with good notice! Lemme know if you have any other concerns about it |
Sounds great! 😎 I'll get working on this asap |
Just been looking at this some more, and it won't be possible to just duplicate the In any case, would it be fair to assume that they are "internal" events anyway? I haven't checked thoroughly but they don't seem to bubble up into userland scope (I could stand corrected here). On the other hand |
A quick note—I decided to revert my changes of these events from camelCase to lowercase because case is still significant in string templates as previously discussed, and I figured there was a chance that there is userland code that might rely on these. So I just added the kebab-case versions instead. |
Hmmm yeah I think we can assume that if an event doens't have an obvious exposure to users then we can consider it |
Sounds good! I'll review tomorrow and see where we're at, and throw you some questions if they arise. I think some documentation might need to be updated too so I will look at that, and I guess if that's everything I'll create a pull request then. |
Sounds good, I'll review as soon as it's up! 🚀 |
This has been released in 0.18.0 Thanks for the contribution! |
emit event name "slideClick" - this violates the event naming convention
72: this.$emit("slideClick", Object.assign({}, e.currentTarget.dataset));
I propose to replace the name of the event "slideClick" with "slideclick"
P.S.Thanks for the interesting gallery !!!
The text was updated successfully, but these errors were encountered: