barbajs / barba

Create badass, fluid and smooth transitions between your website’s pages
https://barba.js.org/
MIT License
11.65k stars 473 forks source link

There is no "after" event that is called after the old container is removed from DOM #455

Closed kaspervdm closed 5 years ago

kaspervdm commented 5 years ago

I was first going to ask this in the Slack channel, but the invitation link in the Readme does not work.

The problem

The "after"-hook gets executed when the old page is still in the DOM. That makes initializing custom javascript prone to errors.

Description

Details

When the transition finishes, I would like to (re)initialize my page javascript. In my code, I search the DOM for elements and set event handlers to buttons etc. I tried all of the available event hooks, but none of them are executed after the old page is completely removed from the DOM. Even the "after" hook is called when both containers are still in the DOM.

Expected behaviour

In the after-event, I should not need to worry that the old container is still in the DOM. I should be able to query the new page only, without having to worry about elements of the previous page.

Actual behaviour

In the after-event, buttons that exist on the old and new page are both in the DOM, making it difficult to set eventlisteners to the correct element.

Code To Reproduce Issue

barba.init({
    transitions: [{
        leave({ current, next, trigger }) {
            return new Promise(resolve => {
                 //some animation here
                TweenMax.to(loader, 1.5, { opacity: "0", ease: Expo.easeInOut, onComplete: function() {
                    resolve();
                }});
            });
        },
        after() {
            console.log(document.querySelectorAll("#js-hamburger")); //this returns two elements instead of one
        }
    }]
});

Environment

luruke commented 5 years ago

Hi @kaspervdm , fix for the slack channel fixed, let me know if that works now.

xavierfoucrier commented 5 years ago

Hi @kaspervdm,

You can remove the current container from the DOM using the enter global hook for example:

barba.hooks.enter(({ current, next }) => {
  current.container.remove();
});

barba.init({
  ...
});

See https://github.com/barbajs/barba/issues/387 for more details. Let me know if it fix your issue. :wink:

kaspervdm commented 5 years ago

Hi @kaspervdm,

You can remove the current container from the DOM using the enter global hook for example:

barba.hooks.enter(({ current, next }) => {
  current.container.remove();
});

barba.init({
  ...
});

See #387 for more details. Let me know if it fix your issue. 😉

I haven't looked at it that way. I could maybe use this in the after-event. However, I would find it strange to interfere with the actions of the library and delete the container myself. It feels like the library should just do this (it already does) and tell me when it's finished doing it (which it doesn't).

In the meanwhile, I have learned that it is probably not a good idea to keep querying the document root to find elements within my page when using barba.

So, instead of using this: document.querySelectorAll("#js-hamburger") I now do this: next.container.querySelectorAll("#js-hamburger")

Only downside is that you have to take this into account when writing your javascript.

xavierfoucrier commented 5 years ago

@kaspervdm,

Of course its better to query the appropriate container... but now I see whats your problem: you are querying an #id element and in fact when using Barba, regarding the container logic, you can have this element duplicated in your document at the same time on the same page... not good!

You need to use a class name for a burger menu, or just extract it from the barba-container so it won't be updated each time you navigate from one page to another.

Be aware with that.

kaspervdm commented 5 years ago

@xavierfoucrier

I agree the way I setup my first project could have been better. The menu could have been placed outside of the barba-wrapper.

It's also true that having two elements with the same ID within the same DOM is not valid. However, this is not something you can prevent when using barba, unless you have full control of all the DOM within your page (might not always be possible if using other libraries).

However, this is not really what created my original issue for me. It's the fact that my querySelectorAll returns two elements instead of one (both the from the current-container and the next-container). In this case I normally would of course use querySelector (instead of querySelectorAll), but that would give me the first element in the DOM, instead of the second (from the next-container).

If the after-event was executed after the current-container was removed from the DOM, my querySelector would have been correct.

xavierfoucrier commented 5 years ago

@kaspervdm the current behavior is that the container is removed at the end of the after hook: we need to be more clear in the documentation about this and I will take the time to update the associated section.

Thanks for highlighting this and taking time to contribute :wink:

thierrymichel commented 5 years ago

Hi @kaspervdm,

Thanks for using barba and contributing. You're right. It makes sense to have before triggered before adding the next container and after triggered after removing the current container.

It's a small change that seems to break nothing… :) @xavierfoucrier, @luruke what do you think?

luruke commented 5 years ago

+1 , go for it!

Il giorno dom 27 ott 2019 alle 11:53 Thierry Michel < notifications@github.com> ha scritto:

Hi @kaspervdm https://github.com/kaspervdm,

Thanks for using barba and contributing. You're right. It makes sense to have before triggered before adding the next container and after triggered after removing the current container.

It's a small change that seems to break nothing… :) @xavierfoucrier https://github.com/xavierfoucrier, @luruke https://github.com/luruke what do you think?

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/barbajs/barba/issues/455?email_source=notifications&email_token=AAAO7DTD2DYLOY4IOQWEDVLQQVQI5A5CNFSM4JBKZFQKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECK3RQI#issuecomment-546683073, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAO7DRWWILPFZEBQFJYGK3QQVQI5ANCNFSM4JBKZFQA .

-- Cheers, Luigi.

http://luruke.com

xavierfoucrier commented 5 years ago

@thierrymichel @luruke agree with that too if its a non-breaking change :wink: