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

Fix: support links with only query parameters #80

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jounimakela
Copy link

This commit resolves an issue where links containing only query parameters without a specified path would incorrectly direct to the root path instead of the current path, unlike standard browser behavior.

Example of a link without a path:

<a href="?lang=en">English</a>

Previously, click_link/2 would direct to root path. In this commit the link correctly retains the current page while applying the query parameters.

@germsvel
Copy link
Owner

germsvel commented Jun 5, 2024

@jounimakela thanks for opening this issue.

I think the problem isn't with phoenix_test itself. We're passing the path to Phoenix.ConntTest.get, which seems to treat that as the root path. Would you mind opening an issue in that repo?

I think if we want to fix that here, we should fix it for all links and not just for when we do a click on static pages. That means we should probably clean up the path that is passed to PhoenixTest.visit/2before sending it to Phoenix.ConnTest.get/2.

You can see that here:

def visit(conn, path) do
case get(conn, path) do

I'm open to the idea of us handling it here (in visit/2) and opening an issue in Phoenix.ConnTest (just to let them know. not sure if they'd fix it). Would you be up for that?

@germsvel
Copy link
Owner

germsvel commented Jun 5, 2024

I think this is also related to #72 (though probably need different fixes). Just noting that here.

@germsvel
Copy link
Owner

germsvel commented Jul 9, 2024

@jounimakela any thoughts on my comment from before? (#80 (comment))

@jounimakela
Copy link
Author

Apologies for my delayed response.

I'm open to the idea of us handling it here (in visit/2) and opening an issue in Phoenix.ConnTest (just to let them know. not sure if they'd fix it). Would you be up for that?

Sounds good! I'll make a proposal for a fix and open an issue.

@germsvel
Copy link
Owner

germsvel commented Jul 9, 2024

Thank you! 🎉 And no apologies needed. This is open source, so we all work on it when we can. 😄

Just wanted to check if this was still something you wanted to fix, and was doing my due diligence here.

This commit resolves an issue where links containing only query
parameters without a specified path would incorrectly direct to the root
path instead of the current path, unlike standard browser behavior.

Example of a link without a path:

    <a href="?lang=en">English</a>

Previously, `click_link/2` would direct to root path. In this commit the
link correctly retains the current page while applying the query
parameters.
@jounimakela
Copy link
Author

Fixing the path is now in PhoenixTest.visit/2. I am not too happy about having the helper function in the top-level module.

I'll also squash commits once we are happy with the changes.

Thoughts?

@germsvel
Copy link
Owner

Thanks for making the change!


I am not too happy about having the helper function in the top-level module.

Yeah, I agree. Something about it doesn't feel great. I think it's because if feels like a band-aid (which it is) on something that Phoenix.ConnTest should handle.

Were you able to open the issue on the phoenix repo? I'd love for this to be fixed there, and we can include the band-aid here until it's fixed. But I don't want to include the band-aid in PhoenixTest forever.

@jounimakela
Copy link
Author

Were you able to open the issue on the phoenix repo? I'd love for this to be fixed there, and we can include the band-aid here until it's fixed. But I don't want to include the band-aid in PhoenixTest forever.

Open now! phoenixframework/phoenix#5877

@jounimakela
Copy link
Author

@germsvel looks like Phoenix.ConnTest.get isn't going to change.

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