Skip to content

[DO NOT MERGE] Add test for transition to with basename #4008

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

Conversation

jochenberger
Copy link
Contributor

@jochenberger jochenberger commented Oct 7, 2016

I added two integration tests that use a basename. I think that they should pass but I'm not sure. See #3839 (comment) for reference. I can try to come up with a fix if you agree that it needs fixing. ;-)

<Router basename={BASENAME}>
<div>
<Link id="target" to={TARGET}>{TARGET}</Link>
<Match pattern={TARGET} render={() => (
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'll probably have to add a leading slash to the pattern here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I merged remix-run/history@df60c49 into history today which automatically takes care of this in the history lib.

), div)

const href = div.querySelector('a').getAttribute('href')
expect(href).toEqual('/foo/bar')
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but the href is /foobar.

@jochenberger
Copy link
Contributor Author

Maybe we need to have a little discussion about relative targets. Is that even something you want to support?
Should <Link to="foo" /> create a link to ${location}/foo?
What about <Link to=".." />? And what about <Link to=".." /> if you are already at the base path?

@jochenberger
Copy link
Contributor Author

See also #3901.

@ryanflorence
Copy link
Member

don't worry about the relative links right now, they are far more tricky than they seem when you add a nesting paradigm in (@mjackson has some PoC work around it) but that first test seems like we have a problem.

@jochenberger
Copy link
Contributor Author

Is there anything I can do to help with this?

@timdorr timdorr added this to the v4.0.0 milestone Nov 1, 2016
@timdorr timdorr added the bug label Nov 1, 2016
@jochenberger
Copy link
Contributor Author

#4122 will probably resolve this.

@pshrmn
Copy link
Contributor

pshrmn commented Nov 17, 2016

@jochenberger It almost does, but not quite. the supports a target that does not start with a slash test fails because there is no parent to join the relative path to in matchPattern, so the <Match> doesn't match. I'll wait to see where they go with <Match>, <Miss>, and <MatchGroup> to determine the appropriate approach, but it would be a simple fix.

@mjackson
Copy link
Member

I commented on our relative strategy in #4122 (comment). Basically, there are two different scenarios for making something "relative": <Route>s and <Link>s. The match.path and match.url strings let us do either one, depending on what we want.

@mjackson mjackson closed this Jan 11, 2017
@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants