fix: consistent event name triggering in components

[DanielRuf]

offcanvas

• rename closed.zf.offcanvas to close.zf.offcanvas
• trigger closed.zf.offcanvas when the transition has ended
• tests: change test for close.zf.offcanvas, add test for closed.zf.off

See #8727

• update / add unit test
• update docs

[ncoden]

Not only a fix as it introduce a new event and sinifically change when closed is triggered.

So LGTM for v6.5 but I would like to make this behavior consistent across all components. So i'm not merging it right now.

[DanielRuf]

Not only a fix as it introduce a new event and sinifically change when closed is triggered.

So LGTM for v6.5 but I would like to make this behavior consistent across all components. So i'm not merging it right now.

Definitely, should I also create an issue and link the changes/PRs for the other components there then?

I remember seeing some issue about (consistent) event naming.

[DanielRuf]

Should I change the title of this PR then?

[ncoden]

Yes ;)

[SassNinja]

I like the new event naming because closed.zf.offcanvas doesn't get triggered anymore while the close transition is still running and the additional close.zf.offcanvas provides more control.

However I'm a bit worried that some users might get broken projects after updating to v6.5 if they rely on closed.zf.offcanvas being triggered immediately...
I'm thinking about a way to add a console.warning to help spotting possible issues but I have no idea how to catch a misuse of the event.

@ncoden @DanielRuf what do you think? There's probably no way to throw a warning if someone is relying on closed.zf.offcanvas before the transitionend, is there?

[ncoden]

@SassNinja I don't think, because the problem would be that they assume that the closed.zf.offcanvas event is triggered before the transition, and we do not know what makes them assuming that.

So everything we can do is a clear migration note in the release.

---

Edit: an other solution would be to keep closed.zf.offcanvas as it, but deprecate it with a warning message and introduce 2 new events. It's basically making the warning message always relevant by making the event always misused ;). I'm not sure it's better.

[DanielRuf]

So everything we can do is a clear migration note in the release.

Definitely.

introduce 2 new events

Not sure how we could also name these.
We should stay consistent with the naming scheme.

[SassNinja]

Edit: an other solution would be to keep closed.zf.offcanvas as it, but deprecate it with a warning message and introduce 2 new events. It's basically making the warning message always relevant by making the event always misused ;). I'm not sure it's better.

Interesting idea what made me think about the possibility of just adding a deprecation warning in 6.5 and hold back the actual PR until 6.6

But I agree: clear migration notes is probably the best solution. I think we can expect people to read the notes before updating their projects.

Not sure how we could also name these.

Yup there is not alternative because close and closed describe it best,

[ncoden]

@SassNinja There is no 6.6 planned as we will start working on v7 ;)

[SassNinja]

Alright, so forget my thought about holding it back.
Then this PR looks good from my side :)

[DanielRuf]

Currently this PR has conflicts.

Also which components do not yet use this event naming scheme (if we know it already) or should I go through all and check them?

[ncoden]

@DanielRuf I don't have them in mind. But even with the list I'll go for manually checking all components where we change properties/attributes or wait for timeout/animation to finish anyway.

[ncoden]

Note: v6.5 was renamed v6.6.

[DanielRuf]

Resolved the conflicts.

[ncoden]

@DanielRuf When your PR is already open, unless if you want to clean the PR, please merge develop (or master) instead of rebasing. We made comments between commits and rebasing will move commits to the bottom, making harder for later to understand what happened.

[ncoden]

1. Off Canvas close() fires close.zf.offcanvas event:
   Error: timeout of 2000ms exceeded. Ensure the done() callback is being called in this test.

2. Off Canvas close() fires closed.zf.offcanvas event:
   Error: timeout of 2000ms exceeded. Ensure the done() callback is being called in this test.

Can you update tests ?

[ncoden]

Merged develop to get full browserstack tests.
Actually I forgot that #11090 was not merged yet...

[ncoden]

@DanielRuf Are you done with this PR ? Is there any component with single events being triggered only before or after something (timeout/transition...) ?

[DanielRuf]

Drilldown, Offcanvas and Reveal trigger closed

Drilldown

    $elem.one(transitionend($elem), function(e){
      $elem.removeClass('is-active is-closing');
    });
        /**
         * Fires when the menu is fully closed.
         * @event Drilldown#closed
         */
    this.$element.trigger('closed.zf.drilldown');

I guess this should be in the callback of transitionend.
The reveal component already triggers it as callback of the animations.

[ncoden]

I guess this shold be in the callback of transitionend.

True.

[DanielRuf]

Updated it accordingly.

[ncoden]

@DanielRuf I took a closer look at the Drilldown test and found a bug in it, that hided a bug in the Drilldown._hideAll() method. I resolved all this and cleaned up Drilldown._hideAll(). See the commits above.

[ncoden]

Merged develop, now tests pass. @DanielRuf are you ok with the changes above?

[DanielRuf]

Merged develop, now tests pass. @DanielRuf are you ok with the changes above?

LGTM

[joeworkman]

This pull request has been mentioned on Foundation Open Source Community. There might be relevant details there:

https://foundation.discourse.group/t/foundation-for-sites-v6-6-0-is-here-farout/30/1