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

supporting anchor links #65

Closed
sethvincent opened this issue Jun 21, 2016 · 11 comments · Fixed by yoshuawuyts/sheet-router#54
Closed

supporting anchor links #65

sethvincent opened this issue Jun 21, 2016 · 11 comments · Fixed by yoshuawuyts/sheet-router#54
Projects
Milestone

Comments

@sethvincent
Copy link
Contributor

Using the default sheet-router/href module results in not being able to jump to anchor links when clicked or when first loading a page with a hash that's meant to point to an anchor somewhere on the page.

Supporting anchor links might not be something we want to have in choo by default, but it would be nice to have docs for it somewhere.

This is also potentially a topic for sheet-router.

Here's where I'm at currently with a solution that overrides the default href behavior:

var choo = require('choo')
var el = choo.view
var app = choo()

app.model({
  namespace: 'app',
  subscriptions: [catchLinks]
})

function catchLinks (send) {
  window.onclick = (e) => {
    const node = (function traverse (node) {
      if (!node) return
      if (node.localName !== 'a') return traverse(node.parentNode)
      if (node.href === undefined) return traverse(node.parentNode)
      if (window.location.host !== node.host) return traverse(node.parentNode)
      return node
    })(e.target)
    if (!node) return
    e.preventDefault()
    var href = node.href

    if (location.pathname !== node.pathname) {
      send('app:location', { location: href.replace(/#$/, '') })
      window.history.pushState(null, null, href)
    } else {
      if (window.location.hash !== node.hash) {
        window.location.hash = node.hash
      }
      scrollToHash(node.hash)
    }
  }
}

app.router(function (route) {
  return [
    route('/', home),
    route('/example', example)
  ]
})

var tree = app.start({ href: false })
document.body.appendChild(tree)

function home (params, state, send) {
  return el`
    <main>
    <h1>home</h1>
    <a href="#home-anchor">home anchor link</a>

    <p>some text</p><p>some text</p><p>some text</p><p>some text</p><p>some text</p><p>some text</p><p>some text</p><p>some text</p><p>some text</p><p>some text</p><p>some text</p>

    <h2 id="home-anchor">home anchor target</h2>
    <a href="/example#example-anchor">link to example anchor</a>
    <p>some text</p><p>some text</p><p>some text</p><p>some text</p><p>some text</p><p>some text</p><p>some text</p><p>some text</p><p>some text</p><p>some text</p><p>some text</p><p>some text</p><p>some text</p><p>some text</p><p>some text</p><p>some text</p><p>some text</p><p>some text</p><p>some text</p><p>some text</p><p>some text</p>
    </main>
  `
}

function example (params, state, send) {
  return el`
    <main>
    <h1>example page</h1>
    <a href="#example-anchor">example anchor link</a>

    <p>some text</p><p>some text</p><p>some text</p><p>some text</p><p>some text</p><p>some text</p><p>some text</p><p>some text</p><p>some text</p><p>some text</p><p>some text</p>

    <h2 id="example-anchor">example anchor target</h2>
    <a href="/#home-anchor">link to home anchor</a>
    <p>some text</p><p>some text</p><p>some text</p><p>some text</p><p>some text</p><p>some text</p><p>some text</p><p>some text</p><p>some text</p><p>some text</p><p>some text</p><p>some text</p><p>some text</p><p>some text</p><p>some text</p><p>some text</p><p>some text</p><p>some text</p><p>some text</p><p>some text</p><p>some text</p>
    </main>
  `
}

function scrollToHash (hash) {
  if (hash) {
    var el = document.querySelector(hash)
    window.scrollTo(0, el.offsetTop)
  }
}
@yoshuawuyts
Copy link
Member

agreed - think this is related to yoshuawuyts/sheet-router#15; we should def think of a way to resolve this

@yoshuawuyts
Copy link
Member

yoshuawuyts commented Jun 30, 2016

We should probs do a document.querySelector("[href='href_value']") and not handle the link if it returns true, cause then we've confirmed it's actually an element on the page and we kinda want the behavior to work as expected. What do you reckon?

@yoshuawuyts
Copy link
Member

Actually yoshuawuyts/sheet-router#15 (comment) might have a better solution. Hmmm, I might need some breathing room from choo for a bit, and while this is definitely a priority it's not super pressing for the stuff I'm doing - any PR to add this would be heaps welcome!

@yoshuawuyts
Copy link
Member

Apparently it's possible - but should probs still fix hah - http://regl.party/api

@sethvincent
Copy link
Contributor Author

Yeah I got that api page working with roughly the code from the example I pasted above 😄 .

I'm starting to wonder if this is a bug that needs to be fixed or if this is reasonable default behavior and we should have a good guide for showing how to override the location object. Or maybe both.

Working on adventuretron today I encountered a similar but different problem, where electron puts file:///whatever/path/to/the/index.html in location.pathname.

Did this to override it:

app.model({
    namespace: 'location',
    state: {
      pathname: '/'
    },
    reducers: {
      pathname: function (data, state) {
        return { pathname: data.pathname }
      }
    },
    subscriptions: [
      function catchLinks (send, done) {
        window.onclick = function (e) {
          var node = (function traverse (node) {
            if (!node) return
            if (node.localName !== 'a') return traverse(node.parentNode)
            if (node.href === undefined) return traverse(node.parentNode)
            if (window.location.host !== node.host) return traverse(node.parentNode)
            return node
          })(e.target)

          if (!node) return
          e.preventDefault()
          var href = node.href.replace('file://', '')

          send('location:pathname', { pathname: href.replace(/#$/, '') }, done)
        }
      }
    ]
  })

I'm thinking a separate package could be created from that traverse function (which i stole from sheetify 😸) plus maybe a little more logic, and in the choo guide we might say something like:

"hey, need to override the location model? you can use whatever-link-catching-module like this, and here's examples for overrides for electron & supporting anchor links using a subscription"

That subscription could then look a little simpler:

var catchLinks = require('whatever-link-catching-module')

subscriptions: [
  function (send, done) {
    catchLinks(function (node, location) {
      var href = node.href.replace('file://', '')
      send('location:pathname', { pathname: href.replace(/#$/, '') }, done)
    })
  }
]

Turns out there are modules that do mostly the right thing with catching links already, like: https://www.npmjs.com/catch-links

@yoshuawuyts yoshuawuyts modified the milestone: 4.0.0 Jul 31, 2016
@yoshuawuyts yoshuawuyts mentioned this issue Jul 31, 2016
24 tasks
@kemitchell
Copy link

FWIW, this is a current need in my work. I'm getting around the limitation with scrollIntoView.

I'll try to find where the change belongs next I can. I gather this cuts across packages, and perhaps a few other checkboxes for 4.0.0.

@MattMcFarland
Copy link
Contributor

So did we decide to fix this as a bug or instead decide this is a browser feature and documentation is needed to explain how to work around it?

@yoshuawuyts
Copy link
Member

Just had a discussion about this with @juliangruber, and we came up with the
following for hash routing:

Inline anchor tags and hash routing cannot co-exist in the same application.
You can choose one, but hash routing should be the default. This is chosen because choo is primarily intended for single page apps, and using hash routing is more in line with that.

Hash routes should be explicit in the application. They're enumerable, and
dynamic hashing can be done through partials:

// enumerable
app.router('/404', [
  [ '/foo', () => {} ],
  [ '/foo#bar', () => {} ],
  [ '/foo#baz', () => {} ],
])
// '/foo#baz' => OK
// '/foo#ohno' => ERROR
// dynamic
app.router('/404', [
  [ '/foo', () => {} ],
  [ '/foo#:baz', () => {} ],
])
// '/foo#baz' => OK
// '/foo#ohno' => OK

An option should be available to enable anchor tag routing, through:
{ anchors: true}.

Browser routing updates can be disabled as a whole through the existing option
of { history: false } - which is useful for widget-like situations.

Thoughts? - should this proposal perhaps be a separate issue?

@kemitchell
Copy link

Had a think.

No applications mixing anchor links and hash routing in a crucial way come to mind. At the same time, IMHO, I've seen plenty of applications using hash routing that should use path routing instead, and then "enable" anchor links again.

Conceptually, I don't see why an application couldn't dynamically differentiate hash-bearing history states with routing implications and hash-bearing history states with anchor links. But practically, that sounds like a nightmare. Not likely worth the effort ... or whatever weight it might add to the bundles.

@MattMcFarland
Copy link
Contributor

I honestly feel like it should use default browser behavior when using an
a-tag, and use the router only when using a js api.

On Tue, Aug 2, 2016 at 5:04 PM, Kyle Mitchell notifications@github.com
wrote:

Had a think.

No applications mixing anchor links and hash routing in a crucial way come
to mind. At the same time, IMHO, I've seen plenty of applications using
hash routing that should use path routing instead, and then "enable" anchor
links again.

Conceptually, I don't see why an application couldn't dynamically
differentiate hash-bearing history states with routing implications and
hash-bearing history states with anchor links. But practically, that sounds
like a nightmare. Not likely worth the effort ... or whatever weight it
might add to the bundles.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#65 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAmDudvsj9KRrng4zcWPlbBbv4oRNY4Sks5qb7DqgaJpZM4I7MX8
.

Matt McFarland

Software Engineer | @docodemore https://twitter.com/docodemore | LinkedIn
http://www.linkedin.com/pub/matthew-mcfarland/6a/b9a/135/en | Mobile:
513.638.8685 | Office: 937.865.6800 xt: 125010

@yoshuawuyts
Copy link
Member

I honestly feel like it should use default browser behavior when using an a-tag, and use the router only when using a js api.

@MattMcFarland Hmm, yeah that does make sense - seems in line with the rest of choo.


Conceptually, I don't see why an application couldn't dynamically differentiate hash-bearing history states with routing implications and hash-bearing history states with anchor links. But practically, that sounds like a nightmare. Not likely worth the effort ... or whatever weight it might add to the bundles.

@kemitchell Yeah, exactly - this is what I initially thought, but yeah I don't feel like the benefits outweigh the costs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Bugs
In Progress
Development

Successfully merging a pull request may close this issue.

4 participants