Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Defaults options and LINK_SELECTOR querySelector #40

Closed
thetoine opened this issue Sep 14, 2018 · 8 comments
Closed

Defaults options and LINK_SELECTOR querySelector #40

thetoine opened this issue Sep 14, 2018 · 8 comments

Comments

@thetoine
Copy link

thetoine commented Sep 14, 2018

Hi,

I'm playing with your lib for a Wordpress custom theme, really liking it so far!

Therefore, I had a couple of issues with my initial setup.

First thing, Wordpress will always output the full http:// URL by default (which is a good SEO practice also). Your default LINK_SELECTOR selector is targeting only relative link.

By default, the LINK_SELECTOR could check if URL is for current origin :

const domain = window.location.origin

const swup = new Swup({
    LINK_SELECTOR: `a[href*="${domain}"]:not([data-no-swup]), a[href^="/"]:not([data-no-swup]), a[href^="#"]:not([data-no-swup]), a[xlink\\:href]`,
})

Also, the class selector except the element to starts with the className, this works too :

const swup = new Swup({
    animationSelector: '[class*="a-"]',
})

It might be a little bit too loose though, any class contain this string part would be selected. The default could be something more specific to the purpose like [class*="a-transition-"].

Anyhow, this just my two cents. If you agree with this, I could fork and send a pull request.

Thanks.

@thetoine thetoine changed the title Defaults options and querySelector Defaults options and LINK_SELECTOR querySelector Sep 14, 2018
@andre94
Copy link

andre94 commented Sep 15, 2018

Hi Thetoine,

For first question see this #4

For second question, and also the first, you can adjust manually the default option like this:

var options = {
    LINK_SELECTOR: 'a', //HERE
    FORM_SELECTOR: 'form[data-swup-form]',
    elements: [
        '#swup'
    ],
    animationSelector: '[class^="a-transition-"]', //HERE
    cache: true,
    pageClassPrefix: '',
    scroll: true,
    debugMode: false,
    preload: true,
    support: true,
    disableIE: false,
    skipPopStateHandling: function(event){
        if (event.state && event.state.source == "swup") {
            return false;
        }
        return true;
    },
}
var swup = new Swup(options) //HERE

Hope this help

@gmrchk
Copy link
Member

gmrchk commented Sep 15, 2018

Hi @thetoine,

The LINK_SELECTOR default setting was once pointed out before. I'm all up for it.

I like the idea about animationSelector option. People are having a hard time understanding the meaning of class^=, or just miss it quite often. I was thinking about including the word swup in the selector, but I guess [class*="transition-"] would do just fine. After all, it's a setting that can be changed... It would be nice if you reflect your changes in the readme as well, but I will go through it when you're done anyway, so it's up to you.

Thanks!

@jcklpe
Copy link

jcklpe commented Sep 22, 2018

just wanted to thank @thetoine for his solution which worked great for me. Thanks!

@gmrchk
Copy link
Member

gmrchk commented Sep 22, 2018

Hi @thetoine, any news on this?

@thetoine
Copy link
Author

@gmrchk sorry, I was way too busy last week on clients job. So you did the change yourself, great !

@dylanfisher
Copy link
Contributor

I don't mean to butt in but I like the convention of including the word swup in the animationSelector by default. I was just thinking about this as I noticed you updated the README with the latest animationSelector example.

@gmrchk
Copy link
Member

gmrchk commented Sep 24, 2018

@thetoine No worries. I was just updating the docs, so I updated the default options in a process so it all fits. Thanks for making me do so!

@dylanfisher transition-* seems pretty descriptive to me, and in case it's overlapping with some internal classes, it's still a default setting that can be changed. Any suggestion including word swup tho? I like the idea, but swup-transition-* is unnecessarily long and versions like swup-a-* seems like something that could be confusing for people.

@jcklpe
Copy link

jcklpe commented Oct 28, 2018

@andre94 @thetoine

Hey guys, wanted to check in with some other people who might have implemented this in WP.

I was looking at adding a Webpack plugin for in-lining my critical path CSS, and it was not able to work because it couldn't access any HTML files. I'm wondering if that means that the preload for swup isn't going to work for wp? I'm continuing to use it because I think the animation stuff is useful but I'm curious how useful it actually is for the actual preloading of data etc. Any thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants