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

Correct handling of new URI API (fixes Deep Link Handling) #660

Merged
merged 2 commits into from
Mar 9, 2024
Merged

Correct handling of new URI API (fixes Deep Link Handling) #660

merged 2 commits into from
Mar 9, 2024

Conversation

Bungeefan
Copy link
Contributor

This PR intends to fix #654 by correcting the usages of uri.toString() with uri.path.

The first commit would technically be sufficient to fix the issue, however, I would strongly recommend to also consider pulling in the second commit as it migrates some of the internal usages to the more appropriate URI API to prevent future mistakes while handling paths.

I also added additional tests to prevent future regressions of this type and tried to restore the behavior as close as possible as it was before bfe7ce3.

Copy link

codecov bot commented Mar 9, 2024

Codecov Report

Attention: Patch coverage is 95.83333% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 97.10%. Comparing base (dcda31e) to head (c70aca7).

Files Patch % Lines
package/lib/src/beamer_delegate.dart 83.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #660      +/-   ##
==========================================
- Coverage   97.11%   97.10%   -0.02%     
==========================================
  Files          14       14              
  Lines         902      897       -5     
==========================================
- Hits          876      871       -5     
  Misses         26       26              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Owner

@slovnicki slovnicki left a comment

Choose a reason for hiding this comment

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

Thank you for the PR @Bungeefan! 💙

LGTM,
I'll proceed with the merge and will publish a new version to pub.

@slovnicki slovnicki merged commit f113906 into slovnicki:master Mar 9, 2024
1 of 3 checks passed
@stan-at-work
Copy link

Thank you for the PR @Bungeefan! 💙

LGTM, I'll proceed with the merge and will publish a new version to pub.

Are you still actively working on the package ?

@slovnicki
Copy link
Owner

Hey @stan-at-work

Yes, I'm still working on the package, but at much slower pace currently. Hopefully, the pace will increase.
I wrote more details in this issue (which is maybe also yours): #659 (comment)

@Sten435
Copy link

Sten435 commented Mar 12, 2024

Hey @stan-at-work

Yes, I'm still working on the package, but at much slower pace currently. Hopefully, the pace will increase. I wrote more details in this issue (which is maybe also yours): #659 (comment)

Correct, thanks for the response

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.

Deep Links are appended instead of replacing the route
4 participants