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

Support filtering DOM nodes that will be processed in href() #26

Closed
wants to merge 2 commits into from
Closed

Conversation

jliuhtonen
Copy link
Contributor

Often it is useful to filter the set of links that will be processed by the router. One common case for this is if you are linking to content inside the same domain that is not a part of your application.

This PR implements an optional filtering function that can be given to href() as a parameter. The function receives a DOM node and the truthiness of it's return value will be used to determine whether to call cb and save the url to history using pushState or not.

This also addresses issue 15.

@jliuhtonen jliuhtonen changed the title Support filtering DOM nodes that href(url) callback will be called with Support filtering DOM nodes that will be processed in href() Jul 8, 2016
@yoshuawuyts
Copy link
Owner

Hmm, I quite like the idea behind this - but think it would lag one step behind the solution proposed in #15 - namely that we can be smart about how / when links should be handled.

@jliuhtonen
Copy link
Contributor Author

Sensible defaults, and ideas suggested in the issue, are definitely good, but sometimes finer control is needed. The need for bypassing processing of links can also be very specific to the app and environment in question, so IMO it is good to provide users the power to choose what gets left out in addition to providing good defaults.

@yoshuawuyts
Copy link
Owner

Hmm, I'd solve that by being explicit in the links themselves and setting a property similar to:

<a href="/foo/bar" data-router="false">
  my cool link
</a>

Feel that if we expose more API surface here, instead of dealing with the problem in the router we're instead pushing it onto users of this project - which is not quite solving it.

@jliuhtonen
Copy link
Contributor Author

jliuhtonen commented Jul 10, 2016

Exposing a DOM node that's a candidate for routing is not that bad in my opinion, since traversing the DOM etc. is done for the user of the library, but I get your point.

A predefined bypass attribute like data-router="false" would also solve the issue in my case, though it would give less control to other users. Then again you can always write your own href implementation if needed. If this approach is preferred, I can change the PR accordingly.

I'd prefer if the attribute would be false by default (so it being absent would mean that it is not active), so we could use boolean attributes and have a name that would be unlikely to clash with other libraries. What do you think? How about data-sheet-no-routing or data-sheet-external-link?

@yoshuawuyts
Copy link
Owner

yoshuawuyts commented Jul 10, 2016

Sounds reasonable, curious to hear other people's opinions on this. Think I'd go for a shorter atrribute though, as 4 words is a bit long. Perhaps:

<a href="/foo/bar" no-routing>my cool link</a>

This wouldn't work in yo-yo / choo straight away tho:

So initially it should be written as:

<a href="/foo/bar" no-routing="true">my cool link</a>

You reckon that sounds reasonable? A PR for that would def be appreciated; we can always discuss the exact keywords. ✨ thanks!

@jliuhtonen
Copy link
Contributor Author

jliuhtonen commented Jul 10, 2016

Okay. We'll have to keep data--prefix though to keep the markup valid HTML 5. Sure, let's do this in its own PR.

@jliuhtonen jliuhtonen closed this Jul 10, 2016
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

Successfully merging this pull request may close these issues.

2 participants