feat: validate location of fragment elements in DOM

hirasso commented 1 year ago

Description

I recently got bitten by the fact that I had fragments defined that were outside of swup's options.containers. This lead to hard-to-understand weirdness between normal and fragment visits.

This PR adds validation so that each fragment for a visit must meet one of these two rules:

It matches one of swup's default containers
It's a descendant of one of swup's default containers

Otherwise the fragment rule will be ignored for the current visit.

How to test:

Markup:

<body>
  <div id="my-test-fragment"></div>
  <div id="swup"><!-- main container --></div>
</body>

JS:

new SwupFragmentPlugin({
    rules: [
        {
            from: '(.*)',
            to: '(.*)',
            containers: ['#my-test-fragment'],
        },
        // ... your other rules
    ],
    debug: true,
});

Checks

[x] The PR is submitted to the master branch
[x] The code was linted before pushing (npm run lint)
[x] The documentation was updated
[x] Includes version bump and changelog update

daun commented 1 year ago

I'm assuming I'm not supposed to look at this yet? Ping me when it's finished 🙃

hirasso commented 1 year ago

pong 🏓 🙂

daun commented 1 year ago

Just to understand — what's the weirdness you've been seeing? Buggy behaviour or difficulties with logging/debugging?

I agree that warning about fragments outside of swup containers is useful, but is there a benefit to absolutely ignoring those fragments instead of merely warning about them? I'm mostly thinking of modals/popups that need to live outside a swup container for styling/accessibility reasons.

hirasso commented 1 year ago

@daun Thanks for asking! :)

The thing I was seeing:

opened a modal fragment that was outside of swup's default containers
clicked a link from inside the modal to somewhere else on the site (full swup visit)
the fragment stayed on the site as it was now outside of swup's control

I'm almost 100% percent sure that there will never be a fragment outside of swup's default containers necessary. But I'm happy to be convinced otherwise. Due to the complexity of the subject I'd love to get together in a call and talk it through.

daun commented 1 year ago

Sure, let's talk it through in a call soon! If you're seeing the issue in the exact case I'm imagining as a counter-point, I'm probably not seeing the whole picture yet.

swup / fragment-plugin

feat: validate location of fragment elements in DOM #55