JS events should be properly camelCased

[jovpet]

Hello, I've noticed a typo error in dropdown-menu component documentation regarding events. In documentation it is marked:

show.zf.dropdownMenu Fires when the new dropdown pane is visible. hide.zf.dropdownMenu Fires when the open menus are closed.

instead events should be without CamelCase like this: show.zf.dropdownmenu and hide.zf.dropdownmenu

[rafibomb]

Thanks for this report! Are you able to submit a Pull Request so we can update this quicker? Or hit https://github.com/zurb/foundation-sites/edit/master/docs/pages/dropdown-menu.md

[jovpet]

Hello @rafibomb. I've tried to use the link you sent, but Preventing FOUC is the last section in dropdown-menu.md. The typo I've noticed is in the JavaScript Reference / Events section. Where could I find that part to edit and make a PR?

[rafibomb]

Oh - gotcha. It's in the auto generated docs, so you can do this through traditional PR in the JS file: https://github.com/zurb/foundation-sites/blob/develop/js/foundation.dropdownMenu.js#L310

[Deckluhm]

The issue isn't with the JS file but with the formatJsEventName Foundation Docs helper: https://github.com/zurb/foundation-docs/blob/master/lib/handlebars/javascript.js#L109

And actually, this isn't even the problem.

--

The problem is some events are camelCased (down.zf.accordionMenu, up.zf.accordionMenu) but some other aren't (show.zf.dropdownmenu, hide.zf.dropdownmenu).

This is sometimes not even consistent inside components: click.zf.accordionMenu vs keydown.zf.accordionmenu

The formatJsEventName helper did his job if we want camelCased event names (e.g. event.zf.someThing).

I personally prefer camelCased event names (or maybe PascalCased) because it's more JS friendly but I guess we need to discuss this before modifying anything.

You may want to read this SO answer about JS event names convention (as defined by W3C): https://stackoverflow.com/a/19071821

[IamManchanda]

camelCase is the right case for JS... No questions about it I concur with @Deckluhm !

[jovpet]

@Deckluhm great clarification of the issue. I didn't notice that some events were camelCased and others weren't. Consistency should be applied, and my vote for camelCase as well!

[andycochran]

We should close this issue by correctly camelCasing all the events, and duplicate the incorrectly cased events so that they log a console message that the event name has been changed. Let's get this in 6.5. /ht @colin-marshall for the fix

[DanielRuf]

This will be fixed by https://github.com/zurb/foundation-docs/pull/24

[colin-marshall]

@DanielRuf doesn't zurb/foundation-docs#24 only change the event name in the docs to be properly camel cased? The problem is there are inconsistencies in the JS code when it came to naming events.

See: https://github.com/zurb/foundation-sites/blob/develop/js/foundation.dropdownMenu.js#L315

So the now the docs are going to display show.zf.dropdownMenu while the actual event is really still show.zf.dropdownmenu.

[DanielRuf]

The new docs will also show show.zf.dropdownmenu which you can verify by applying the patch from the merged PR to node_modules/foundation-sites and running gulp docs or gulp docs:all.

I guess we mixed several things here and I got confused =/

[DanielRuf]

They will all be lowercased like they are and always were internally triggered. I was confused by the Dropdown Menu component, all events should be camelCase. Only Dropdown Menu used the wrong event names.

[DanielRuf]

If the events in the js docs are not consistent yet, can you open an issue? We can then togethet check all trigger statements.

[DanielRuf]

instead events should be without CamelCase like this: show.zf.dropdownmenu and hide.zf.dropdownmenu

The title suggests something different.

How should we decide to go on? The formatJsEventName handlebars helper is meant to make it lowerCamelcased or just all lowercase?

https://github.com/zurb/foundation-docs/blob/master/lib/handlebars/javascript.js#L104

[DanielRuf]

cc @ncoden

[colin-marshall]

@ncoden are we having event names be all lower case or camel case? I noticed in the recent foundation-docs update that you are now using this.memberof to get the plugin name, which prints the event as camel case in the docs. The problem is we have events named inconsistently (some all lowercase like hide.zf.dropdownmenu) when they are triggered so they don't match the camel casing in the recent docs update.

Changing the name in the trigger to be camel case is somewhat of a breaking change, so I'm not sure where to go from here until we hit 6.5 where it's ok to break.

[DanielRuf]

As far as I can see trigger in the components sometimes uses lowerCamelcase and sometimes lowercase.

See Dropdown Menu show.zf.dropdownmenu vs Accordion Menu down.zf.accordionMenu vs Responsive Toggle toggled.zf.responsiveToggle

[DanielRuf]

So It seems we have to revert the change from https://github.com/zurb/foundation-docs/pull/24, and change the triggered event name in Dropdown Menu and others.~ This is fine =)

[ncoden]

Be consistent in the event naming is good, but not enough useful to be a breaking change, not in a pre-v7 version. Also I don't understand the issue with zurb/foundation-docs#24. Does the format helper make the event name or namespace incorrect ?

[DanielRuf]

Hm, I'll check it. It seems only Dropdown Menu is the issue.

[DanielRuf]

The change in the docs was correct. We just always kept the event names in the Dropdown Menu as they were and tried to solve it in the docs which was not right. I've created a PR to finally trigger the right event names in the Dropdown Menu component.

Sorry for the confusion but now I have a much better overview after reviewing more components regarding the event naming scheme.

[DanielRuf]

Or should I move this to the v7 milestone?

[colin-marshall]

@ncoden the docs change was fine overall, but it displays the dropdown menu event names camel cases when they are all lowercase.

For, now we could add a conditional to the formatJsEventName helper so that it changes only dropdown menu events to all lowercase.

[colin-marshall]

I don’t see a problem with making it a breaking change in 6.5.

[ncoden]

Ok I think I finally understand the problem:

For a plugin named:

title: Plugin Name

Event are described with:

// @event PluginName#eventname

And trigger:

.trigger('eventname.zf.pluginName')

But OffCanvas and DropdownMenu trigger x.zf.offcanvas and x.zf.dropdownmenu... So or the comment or the triggered event is incorrect.

According to the JSDoc documentation, @event expect a classname (so in PascalCase). But there is an additional parameter :event which allow to specify a "namespace". I am not able to figure out if it is an "event namespace" if it has a particular role within JSDoc.

So, or we continue to investigate on the namespace parameter, or we change the events to camel case. For the 2nd option, retrieve the event listeners to display a deprecation notice when the event is triggered. So we can trigger 2 events and only document new last one.

But honestly I don't think it worth it (to change code and add deprecation notices) for what is basically a documentation problem and a tiny inconsistency, not without v6.6 planned.

[colin-marshall]

@ncoden quick and dirty fix for this so that docs match event triggers across the board is edit the formatJsEventName function in foundation-docs/lib/handlebars/javascript.js to this:

handlebars.registerHelper('formatJsEventName', function(name, title) {
  if (title === 'DropdownMenu' || title === 'OffCanvas') {
    title = title.toLowerCase();
  } else {
    title = title.charAt(0).toLowerCase() + title.slice(1);
  }
  return format('{0}.zf.{1}', [name, title.replace(/(-| )/, '')]);
});

[ncoden]

@colin-marshall Hahaha

No.

Please, no.

[colin-marshall]

Ya it sucks but it was just a temporary suggestion until you figure out the proper way to do it with JSDoc namespaces.

It's better than having the Docs show incorrect information.

[ncoden]

Best case: we use JSDoc namespaces. Worst case, we deprecate events and create lowercase ones.

[DanielRuf]

At least, I fear so, we have to break something to be fully consistent in the future.

[ncoden]

So after further investigations:

• This is not a Documentation issue since this has been resolved in https://github.com/zurb/foundation-docs/pull/24. Dropdownmenu is fixed and only OffCanvas doc is out of date. It's only a consistency issue.
• SuperCollider rely on an old version of jsdoc-api which does not support event namespaces, are JSDoc namespaces does not seem to have the same purpose as "jQuery event namespaces".

Also, since we are already deprecating events at https://github.com/zurb/foundation-sites/pull/10949, I think we can try to clean our whole event API for v6.5.0 without caring too much about backwards compatibility, as long as we provide a simple migration guide.

[ncoden]

@DanielRuf What do you think ?

If we're all Ok with this you can continue your work on https://github.com/zurb/foundation-sites/pull/10965 (and add offCanvas and smoothScroll to it) 😉

[DanielRuf]

I would finish the PR and also adjust the other two components to have some consistency so far.

For 6.5.0 we definitely need an overhauled event system and documentation with more consistent name schemes.