Vue-carousel: Bug: event name casing is not standard

[stokaboka]

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 !!!

[quinnlangille]

Hey @stokaboka, good catch I'll switch this to a bug report

To resolve, we should:

1) Convert the slideClick event to be all lowercase, as per vuejs standard
2) Audit all other events to make sure they are also up to this standard

Should be a super easy PR if someone is looking to get their feet wet!

[darraghenright]

Hi @quinnlangille

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 slideClick to slideclick.

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:

• pageChange
• transitionStart
• transitionEnd

So these would have to be changed as well. Vue's Event Names documentation recommends that events should be in kebab-case; i.e:

• page-change
• transition-start
• transition-end

But this is inconsistent with the other existing lowercased events navigationclick, paginationclick and slideclick.

Therefore I guess it might be more prudent to change events in camelCase events to lowercase:

• pagechange
• transitionstart
• transitionend

Let me know if that's your preference and I can submit a PR!

[darraghenright]

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.

[quinnlangille]

Hey @darraghenright, thanks for taking on the issue! I would be fine with non-hyphenated (pagechange vs page-change) but, if you'd like to migrate all events to hyphenated versions feel free. I think meeting the vue style guide is a direction we should be taking. It would be good to maintain backwards compatibility here if we can!

Let me know if you have any questions :octocat:

[darraghenright]

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 pageChange event for some future, backwards-incompatible release. Or is that a naive reading of the requirement? 😄

[darraghenright]

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 camelCase and PascalCase as their lowercase variants would be expected.

However, in string templates, event listeners _are_ case-sensitive. So this could mean that there are users out there successfully listening for emitted events pageChange, transitionStart, transitionEnd etc.

Just wondering now, if there would be unitended consequences in changing the existing camelCase events to lowercase for one specific case. I don't know if many people use DOM templates - I don't but I suspect that it's not uncommon.

[quinnlangille]

@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

[darraghenright]

Sounds great! :sunglasses: I'll get working on this asap

[darraghenright]

Just been looking at this some more, and it won't be possible to just duplicate the navigationclick and paginationclick events with their kebab-case counterparts because they are used internally to trigger modifications to the page number. One of the navigation tests fails as a result of this. I'll have a look of what can be done there.

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 slideclick pageChange, transitionStart and transitionEnd seem pretty safe, because they are not used internally and don't trigger changes to the state. I'll commit kebab-case version of those and update the Vue Play examples.

[darraghenright]

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.

[quinnlangille]

Hmmm yeah I think we can assume that if an event doens't have an obvious exposure to users then we can consider it internal. I think as long as we're standardized and backwards compatibility is maintained then we should be on the right track. Once we issue and move past the deprecation notice, we'll be free to make whatever change is necessary - until then, let's play it safe! Thanks for looking into this @darraghenright, I'll be available all week so feel free to message if you have any other questions :octocat:

[darraghenright]

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.

[quinnlangille]

Sounds good, I'll review as soon as it's up! 🚀

[quinnlangille]

This has been released in 0.18.0 :octocat: Thanks for the contribution!