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 prefixed paths #329

Merged
merged 5 commits into from
Oct 6, 2015
Merged

Support prefixed paths #329

merged 5 commits into from
Oct 6, 2015

Conversation

mquandalle
Copy link
Contributor

Fixes #315


Currently, this does not work. I'm having some trouble understanding pagejs behaviour and internals distinctions between a canonicalPath and path, and why the URL isn’t correctly modified even when pagejs is internally aware of the base path.

@arunoda
Copy link
Contributor

arunoda commented Sep 21, 2015

Not sure. I'll have a look at this.

@mquandalle
Copy link
Contributor Author

Hum, actually I think this was only an issue with the tests.

@mquandalle
Copy link
Contributor Author

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 :-)

@mquandalle mquandalle changed the title [WIP] Support base paths Support base paths Sep 21, 2015
@arunoda
Copy link
Contributor

arunoda commented Sep 21, 2015

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,
Copy link
Contributor Author

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?

Copy link
Contributor

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.

@arunoda
Copy link
Contributor

arunoda commented Sep 21, 2015

@pahans could you run these tests on some older IE version?

@mquandalle
Copy link
Contributor Author

And by the way, I haven’t implemented anything to support server side rendering under a prefixed path.

@arunoda
Copy link
Contributor

arunoda commented Sep 21, 2015

That's fine. I'll take care of that. Normally, SSR thing is behind few version from this one and we'll merge carefully.

@arunoda
Copy link
Contributor

arunoda commented Sep 21, 2015

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)

@mquandalle
Copy link
Contributor Author

I've just tested this application as well, calling FlowRouter.go('/') or FlowRouter.go('/abcPostId') from the browser console is working. The links don't work because they point to the wrong path (I guess I should update the pathFor method). Also the URL pattern matching isn't working on the first page load (but again works on further calls using FlowRouter.go), not sure if you have a clue about what to do in this case?

@ghost ghost mentioned this pull request Sep 23, 2015
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.
@mquandalle
Copy link
Contributor Author

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?

@mquandalle
Copy link
Contributor Author

Tested on Wekan and it works smoothly. I would say this is ready to merge.

@mquandalle mquandalle changed the title Support base paths Support prefixed paths Oct 5, 2015
arunoda added a commit that referenced this pull request Oct 6, 2015
@arunoda arunoda merged commit 62f7f4a into kadirahq:master Oct 6, 2015
@arunoda
Copy link
Contributor

arunoda commented Oct 6, 2015

Thanks. This is working pretty great. 👍

@arunoda
Copy link
Contributor

arunoda commented Oct 6, 2015

Released as: v2.7.0

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