-
Notifications
You must be signed in to change notification settings - Fork 192
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 prefixed paths #329
Conversation
Not sure. I'll have a look at this. |
Hum, actually I think this was only an issue with the tests. |
Ok, now it works and I've added two tests. I encourage a careful review because it’s 3am here and I'm a bit tired :-) |
Ha ha. Get some sleep :) |
@@ -364,7 +369,13 @@ Router.prototype.initialize = function(options) { | |||
// in unpredicatable manner. See #168 | |||
// this is the default behaviour and we need keep it like that | |||
// we are doing a hack. see .path() | |||
this._page({decodeURLComponents: true, hashbang: !!options.hashbang}); | |||
this._page({ | |||
basePath: this._basePath, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this is useful, the below call to this._page.base
seems to be enough on my browser but maybe basePath
is used for compatibility with old IE?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to set this._page.base()
as well? I try to run this on IE.
@pahans could you run these tests on some older IE version? |
And by the way, I haven’t implemented anything to support server side rendering under a prefixed path. |
That's fine. I'll take care of that. Normally, SSR thing is behind few version from this one and we'll merge carefully. |
I tried with this app and some other our apps and this seems like not working. May be we need to work on adding prefixes manually. (I mean in the FlowRouter level, not directly on page.js level) |
I've just tested this application as well, calling |
We need to set the `._page.base` path before calling the `_page` routing function. This fixes the first page routing when a page prefix is set.
Hey @arunoda, I've done some slight modifications on this PR and successfully got it working on the above flow-router example application. Could you try https://github.com/mquandalle/flow-router-guide-example/tree/prefixed-paths? |
Tested on Wekan and it works smoothly. I would say this is ready to merge. |
Thanks. This is working pretty great. 👍 |
Released as: |
Fixes #315
Currently, this does not work. I'm having some trouble understanding
pagejs
behaviour and internals distinctions between acanonicalPath
andpath
, and why the URL isn’t correctly modified even whenpagejs
is internally aware of the base path.