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

fix(rxApp): Fix #446; demo app nav not highligting when active #458

Merged
merged 1 commit into from Aug 27, 2014

Conversation

Projects
None yet
5 participants
@klamping
Copy link
Contributor

klamping commented Aug 15, 2014

Fix to resolve #446

FYI rxBillingSearch changes are just getting rid of the alien whitespace characters that @halkon added.
ç∂ßçßß稆ø®      

@comeatmebro

This comment has been minimized.

Copy link

comeatmebro commented Aug 15, 2014

@parlarjb

This comment has been minimized.

Copy link
Contributor

parlarjb commented Aug 15, 2014

@parlarjb

This comment has been minimized.

Copy link
Contributor

parlarjb commented Aug 18, 2014

This doesn't work against Cloud Atlas. Cloud uses <base href="/cloud/"> as described here: http://www.verdantrefuge.com/writing/2013/angularjs-changing-app-path-base-element/

But $location.path() is just returning (for example) /hub_cap/servers" instead of /cloud/hub_cap/servers". The route "items" are getting the /cloud prepended, but then not matching because $location.path() isn't included /cloud/

Sigh.

@hussamd

This comment has been minimized.

Copy link
Contributor

hussamd commented Aug 18, 2014

We should keep absUrl() for the basePath. It will return the path prefix defined in <base href="" /> of any given app.

Normally single page apps run from the root directory of the web server. When they aren't run from root and are put into a sub-directory and given their own document roots; a server side app doesn't know that it's being run from a sub-directory. The document root for each app is set and all URLs are relative from that document root.

However, in the case of a client side app, should we know where it resides (rather than assume that it is still in the root directory by just using $location.path()'s result)? Without this information it may get hard to match relative URLs (the routes within a given app) with the currently running app's URL (the basePath or absUrl()).

@parlarjb

This comment has been minimized.

Copy link
Contributor

parlarjb commented Aug 18, 2014

@klamping

This comment has been minimized.

Copy link
Contributor

klamping commented Aug 18, 2014

Good catch on the cloud thing. Is there an easy way to create a unit test for the 'base' scenario?

@hussamd

This comment has been minimized.

Copy link
Contributor

hussamd commented Aug 18, 2014

Yes, we will add a test case for this.

@comeatmebro

This comment has been minimized.

Copy link

comeatmebro commented Aug 20, 2014

@hussamd

This comment has been minimized.

Copy link
Contributor

hussamd commented Aug 20, 2014

I am going to pull this down and look at the demo page. I had gotten busy with other things, but I have cycles today!

@klamping

This comment has been minimized.

Copy link
Contributor

klamping commented Aug 20, 2014

Pulling over comment from #459

Just tested this locally and am running into the issue I mentioned:
URL of page: http://localhost:9001/#/component/rxApp

nav item JSON:

{
    href: '/rxApp',
    linkText: 'rxApp'
}

With the nav item getting highlighted:
image

FYI, I did have to add that nav item in to the demo code to create the issue.

that nav item should not highlight when on /component/rxApp, but b/c it only looks at '/rxApp', it highlights it. i think we might want to check for a tag in our JS, instead of just assuming anything before the '/rxApp' is part of the overall application URL and can be ignored

@klamping

This comment has been minimized.

Copy link
Contributor

klamping commented Aug 25, 2014

Current status on this is that I'm still looking at solutions for the bug I mentioned. Will either have a viable solution by EOD today or merge as is and open an issue.

@hussamd

This comment has been minimized.

Copy link
Contributor

hussamd commented Aug 25, 2014

Merge as is 👍

@klamping klamping force-pushed the 446-active-url branch from e6c20ad to 164ae6b Aug 25, 2014

@klamping

This comment has been minimized.

Copy link
Contributor

klamping commented Aug 25, 2014

I believe I solved the problem for both conditions. Please review when you can, in the meantime I'll write up some test cases for the various scenarios.

@klamping

This comment has been minimized.

Copy link
Contributor

klamping commented Aug 26, 2014

@parlarjb @hussamd added tests and fixed logic. Mind looking over again?

@comeatmebro

This comment has been minimized.

Copy link

comeatmebro commented Aug 26, 2014

@parlarjb

This comment has been minimized.

Copy link
Contributor

parlarjb commented Aug 27, 2014

LGTM

FYI, I tried this out with Cloud Atlas, and it worked fine

@hussamd

This comment has been minimized.

Copy link
Contributor

hussamd commented Aug 27, 2014

Thank you,.

FYI I tried this with nothing, but I am happy with the solution. Please merge.

@comeatmebro

This comment has been minimized.

Copy link

comeatmebro commented Aug 27, 2014

parlarjb added a commit that referenced this pull request Aug 27, 2014

Merge pull request #458 from rackerlabs/446-active-url
fix(rxApp): Fix #446; demo app nav not highligting when active

@parlarjb parlarjb merged commit 8228d82 into master Aug 27, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@parlarjb parlarjb deleted the 446-active-url branch Aug 27, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment