Skip to content
This repository has been archived by the owner on Aug 30, 2021. It is now read-only.

fix(core): Remove the <base> tag (continues #1230) #1544

Merged
merged 3 commits into from
Oct 12, 2016

Conversation

mleanos
Copy link
Member

@mleanos mleanos commented Oct 6, 2016

@0x24a537r9

Fixes #1224. Everything looks good in the browser and server/e2e tests, but unfortunately the unit tests are broken, and I'll probably need some guidance from someone more familiar with Angular/Karma.

This is a continuation of #1230, in an attempt to finalize the changes by fixing the broken tests.

NOTE: The fixes to the client-side tests aren't ideal. At the moment, they will suffice. This comment,
among others in that issue, led me to choose this method as the fix to
avoid having to change any core code.

@coveralls
Copy link

coveralls commented Oct 6, 2016

Coverage Status

Coverage remained the same at 73.036% when pulling a43700bbcb50821914671cff615dc11b83a17afc on mleanos:remove-base-tag into 73a7c2c on meanjs:master.

@lirantal
Copy link
Member

lirantal commented Oct 6, 2016

@mleanos thanks.
this PR supercedes the other base tag PR, right?

@mleanos
Copy link
Member Author

mleanos commented Oct 6, 2016

@lirantal Yes. We can close to the other.

@lirantal
Copy link
Member

lirantal commented Oct 6, 2016

Ok I'll close the older one. Can you rebase your branch here with the latest changes?

@mleanos
Copy link
Member Author

mleanos commented Oct 7, 2016

I'm waiting for #1547 & #1532 to be merged, before I rebase. Those PR's should go in first, and will cause conflicts with this. I'll fix as soon as they go in.

0x24a537r9 and others added 3 commits October 10, 2016 16:05
Updated tests to account for new "/" prefix in the path.
Fixes the client-side tests after the removal of the <base/> tag from
the main layout.

These fixes aren't ideal. At the moment, they will suffice. This comment
(angular-ui/ui-router#212 (comment)),
among others in that issue, led me to choose this method as the fix to
avoid having to change any other core code.
@mleanos
Copy link
Member Author

mleanos commented Oct 11, 2016

@lirantal This has been rebased.

@lirantal
Copy link
Member

It's a big change through a lot of files so I want to test it a bit locally to confirm it's ok not breaking anything first and then I'll merge.

@mleanos
Copy link
Member Author

mleanos commented Oct 11, 2016

@lirantal When you merge, don't squash. I noticed with the other PR that I took over, the commits from the original contributor were squashed into mine; thus, the original author's commits aren't reflected in the history.

If you'd prefer, I can squash my commits down to 1 so there would be only two going in with this merge. Either way, it's not a big deal to me but we should try to at least keep @0x24a537r9's commits.

@lirantal
Copy link
Member

Of course, no problems.

@lirantal
Copy link
Member

Seems to work great, thanks @mleanos and @0x24a537r9 for all the help!

@lirantal lirantal merged commit d824224 into meanjs:master Oct 12, 2016
@abumere
Copy link

abumere commented Oct 22, 2016

Looks like removing the tag also broke the ability to add a URL in the config/assets/default or production files.

Normally you could add a URL as a css dependency (lets say to a google font) but now it appends localhost to the beginning of the URL

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

<base href="/"> breaks SVG IRIs and element CSS URLs
5 participants